What is wrong with this code?

[Prabh]

Hello,

I am wondering whats wrong with the following code. It is crashing on
Linux, but works fine on AIX.

The function basically get DbNm@Srvr string and then return DbNm and
Srvr back to the calling program.
#define IDFR_VAL_SIZE 100

int rmlPvtGetDbNm(char *sDb, char *sSrvr, char *sDbNm)
{
char *sTok;

memset(sDb, '\0', IDFR_VAL_SIZE);
memset(sSrvr, '\0', IDFR_VAL_SIZE);

sTok = malloc(IDFR_VAL_SIZE);

sTok = strtok( sDbNm, "@" );
strcpy(sDb, sTok);

sTok = strtok(NULL, "@" );
strcpy(sSrvr, sTok);

free(sTok);

if (memcmp(sDb, "NO-DB", 5) ==0)
{
return 1;
}
return 0;

}
with regards,

prabh

[Jack Klein]

On 28 Jul 2003 20:37:22 -0700, pr*******@videotron.ca (Prabh) wrote in
comp.lang.c:

Hello,

I am wondering whats wrong with the following code. It is crashing on
Linux, but works fine on AIX.

The function basically get DbNm@Srvr string and then return DbNm and
Srvr back to the calling program.
#define IDFR_VAL_SIZE 100

int rmlPvtGetDbNm(char *sDb, char *sSrvr, char *sDbNm)
{
char *sTok;

memset(sDb, '\0', IDFR_VAL_SIZE);
memset(sSrvr, '\0', IDFR_VAL_SIZE);
Where are the arrays of characters that sDb and sSrvr point to
allocated or defined? If either of them actually points to less than
100 characters, you are writing past into memory you don't own, a good
way to crash.
sTok = malloc(IDFR_VAL_SIZE);

sTok = strtok( sDbNm, "@" );
This is a memory leak. You allocate 100 characters to sTok, then
throw away the allocated memory by assigning the return value of
strtok() to it. Poof, 100 bytes leaked and gone.
strcpy(sDb, sTok);
strtok() can return a null pointer. Passing a null pointer as the
second argument to strcpy() is undefined behavior and can cause a
crash. Alternatively, if the first '@' character is more than 100
characters into sDbNm, you'll overflow
sTok = strtok(NULL, "@" );
strcpy(sSrvr, sTok);
Likewise, strtok() can return a null pointer here. Never use the
value returned by strtok() without testing for NULL.
free(sTok);
This is almost certainly what is causing your crash. You allocated
memory to sTok, then you lost the pointer to it by overwriting it with
the return value of strtok(), not once, but twice.

So now you are trying to free sTok. But the value you are passing to
free() is not the pointer value returned by malloc().
if (memcmp(sDb, "NO-DB", 5) ==0)
{
return 1;
}
return 0;

}
with regards,

prabh

--
Jack Klein
Home: http://JK-Technology.Com
FAQs for
comp.lang.c http://www.eskimo.com/~scs/C-faq/top.html
comp.lang.c++ http://www.parashift.com/c++-faq-lite/
alt.comp.lang.learn.c-c++ ftp://snurse-l.org/pub/acllc-c++/faq

[James Antill]

On Mon, 28 Jul 2003 20:37:22 -0700, Prabh wrote:

Hello,

I am wondering whats wrong with the following code. It is crashing on
Linux, but works fine on AIX.
I'm sure IBM would help you fix^H^H^H port your code.
The function basically get DbNm@Srvr string and then return DbNm and
Srvr back to the calling program.

#define IDFR_VAL_SIZE 100

int rmlPvtGetDbNm(char *sDb, char *sSrvr, char *sDbNm)
{
char *sTok;

memset(sDb, '\0', IDFR_VAL_SIZE);
memset(sSrvr, '\0', IDFR_VAL_SIZE);

sTok = malloc(IDFR_VAL_SIZE);

sTok = strtok( sDbNm, "@" );
You've just lost the memory from the malloc.
strcpy(sDb, sTok);

sTok = strtok(NULL, "@" );
strcpy(sSrvr, sTok);

free(sTok);
Now you are free'ing a random offset into sDbNm.
if (memcmp(sDb, "NO-DB", 5) ==0)
{
return 1;
}
return 0;

}

You also assume a lot of things about the sizes of the objects and don't
check any return values (and the memcmp() at the end implies to me that
the strtok()'s could fail).

--
James Antill -- ja***@and.org
Need an efficent and powerful string library for C?
http://www.and.org/vstr/

[Martin Ambuhl]

Prabh wrote:

Hello,

I am wondering whats wrong with the following code. It is crashing on
Linux, but works fine on AIX.

The function basically get DbNm@Srvr string and then return DbNm and
Srvr back to the calling program.
#define IDFR_VAL_SIZE 100

int rmlPvtGetDbNm(char *sDb, char *sSrvr, char *sDbNm)
{
char *sTok;

memset(sDb, '\0', IDFR_VAL_SIZE);
memset(sSrvr, '\0', IDFR_VAL_SIZE);

sTok = malloc(IDFR_VAL_SIZE);

sTok = strtok( sDbNm, "@" );
strcpy(sDb, sTok);

sTok = strtok(NULL, "@" );
strcpy(sSrvr, sTok);

free(sTok);

if (memcmp(sDb, "NO-DB", 5) ==0)
{
return 1;
}
return 0;

}

You overwrite the value returned from malloc() not once, but three
times! No wonder free() gets confused. Even worse, there was never any
need to malloc() or free() anything. See the code below to see just how
much simpler your code could have been. Why your code "worked" before is
anyone's guess. One correct way of your code's working is to crash.
#include <string.h>
#include <stdio.h>

#define IDFR_VAL_SIZE 100

int rmlPvtGetDbNm(char *sDb, char *sSrvr, char *sDbNm)
{
char *sTok;

if ((sTok = strtok(sDbNm, "@")))
strcpy(sDb, sTok);

if ((sTok = strtok(NULL, "@")))
strcpy(sSrvr, sTok);

return (memcmp(sDb, "NO-DB", 5) == 0);
}
int main(void)
{
char input[IDFR_VAL_SIZE] = "DbNm@Srvr";
char dbnm[IDFR_VAL_SIZE], srvr[IDFR_VAL_SIZE];
printf("return value was: %d\n", rmlPvtGetDbNm(dbnm, srvr, input));
printf("dbnm: %s, srvr: %s\n", dbnm, srvr);
return 0;
}