Your opinions on some code, please

[Emmet Caulfield]

In the course of examining the code for an Internet-connected
authentication server, I came across the following code (twice in one
function with different constants in the "if") in a file of some 2000
non-comment lines written by a "C" expert:

if( elem->tag == SOME_MANIFEST_CONSTANT ) /* Alt */
{
char *tempValue = malloc(32);
strcpy(tempValue, elem->value);
while( tempValue[0] == '0' )
{
tempValue++;
}
some_api_funtion(tempValue); /* Alt */
}

(The comments are mine and denote lines that have been altered
for brevity or clarity)

I wonder if the denizens of comp.lang.c would care to comment?

Emmet.

 

[Servé La]

In the course of examining the code for an Internet-connected
authentication server, I came across the following code (twice in one
function with different constants in the "if") in a file of some 2000
non-comment lines written by a "C" expert:

if( elem->tag == SOME_MANIFEST_CONSTANT ) /* Alt */
{
char *tempValue = malloc(32);
strcpy(tempValue, elem->value);
while( tempValue[0] == '0' )
{
tempValue++;
}
some_api_funtion(tempValue); /* Alt */
}

(The comments are mine and denote lines that have been altered
for brevity or clarity)

I wonder if the denizens of comp.lang.c would care to comment?

not checking the return value of malloc.
not checking for buffer overflow in strcpy
tempValue not freed (I can't believe such easy to avoid mem leaks are
written)

 

[Jens.Toerring]

In the course of examining the code for an Internet-connected
authentication server, I came across the following code (twice in one
function with different constants in the "if") in a file of some 2000
non-comment lines written by a "C" expert:

if( elem->tag == SOME_MANIFEST_CONSTANT ) /* Alt */
{
char *tempValue = malloc(32);

Missing check for return value of malloc(). Using magic numbers is
bad style.

strcpy(tempValue, elem->value);

Potential buffer overrun when the string in elem->value (lets hope
it's one...) is longer than 31 chars. Hopefully it's checked some-
where else that this condition is always satisfied.

while( tempValue[0] == '0' )
{
tempValue++;
}

This looks really stupid since afterwards it's difficult to determine
what tempValue originally was to be able to call free() on it - since
the skipped '0' chars are also in elem->value (lets hope it won't get
changed somewhere before that) it should be possible to get back at the
original value of tempValue but why not store it somewhere safely? I
also would prefer to write the 'tempValue[0]' as '*tempValue'.

some_api_funtion(tempValue); /* Alt */

Lets just hope in that function nothing gets appended to the
tempValue buffer because that can't be done safely...

And now we are missing a free() on tempValue (after carefully
correcting it to have the value of what was returned by malloc()).
A perfect specimen of a memory leak. The only way around that would
be when tempValue gets free()ed in some_api_funtion() (but to do
that safely requires that the function knows about the elem->value
string). But that would be extremely bad style for several reasons.

}

Regards, Jens

 

[Richard Bos]

In the course of examining the code for an Internet-connected
authentication server, I came across the following code (twice in one
function with different constants in the "if") in a file of some 2000
non-comment lines written by a "C" expert:

if( elem->tag == SOME_MANIFEST_CONSTANT ) /* Alt */
{
char *tempValue = malloc(32);

And if malloc() fails, you scribble through the null pointer.
The magic number 32 is also hardly advisable.

strcpy(tempValue, elem->value);

This may look like a mistake, but it's quite possible that
SOME_MANIFEST_CONSTANT means that there will never be more than 32 (or
more solidly, SOME_MANIFEST_SIZE) chars in elem->value, and that this
has been checked in the surrounding code. If not:

*tempValue='\0';
strncat(tempValue, elem->value, SOME_MANIFEST_SIZE-1);

while( tempValue[0] == '0' )
{
tempValue++;
}
some_api_funtion(tempValue); /* Alt */

And you lose the original pointer that malloc() returned, so you cannot
free() it. Even if some_api_function puts tempValue into a global
container (tree, list, whatever), you have no way of knowing whether it
is the original pointer or one a few characters further on, so you can't
pass it to free(). Result: memory leak.

}

Tell me, which inadvisable product's code did you find this in?

Richard

 

[Alex Fraser]

In the course of examining the code for an Internet-connected
authentication server, I came across the following code (twice in one
function with different constants in the "if") in a file of some 2000
non-comment lines written by a "C" expert:

if( elem->tag == SOME_MANIFEST_CONSTANT ) /* Alt */
{
char *tempValue = malloc(32);
strcpy(tempValue, elem->value);
while( tempValue[0] == '0' )
{
tempValue++;
}
some_api_funtion(tempValue); /* Alt */
}

(The comments are mine and denote lines that have been altered
for brevity or clarity)

I wonder if the denizens of comp.lang.c would care to comment?

Unchecked return from malloc().
Small, fixed-size malloc() is generally pointless - might as well just use
an auto buffer.
Potential strcpy() buffer overflow.
tempValue cannot be free()d, because the while loop changes the value in a
non-reversible way.
Copies the whole thing then skips any leading '0' characters - could skip
them first and copy only the remainder.
If some_api_function() doesn't modify the pointed-to buffer then there's no
need to copy at all.
I won't bother with the style/layout issues because, aside from being
religious, they are minor by comparison.

If some_api_function() modifies the buffer, I'd probably use:

if (elem->tag == SOME_MANIFEST_CONSTANT) {
const char *p;
char *temp;

for (p = elem->value; *p == '0'; ++p)
;
temp = malloc(strlen(p) + 1);
if (!temp) abort(); /* or something more appropriate */
strcpy(temp, p);
some_api_function(temp);
free(temp);
}

If not:

if (elem->tag == SOME_MANIFEST_CONSTANT) {
const char *p;
for (p = elem->value; *p == '0'; ++p)
;
some_api_function(p);
}

Or (since you mention the code is duplicated except for the if test) put the
body of the if into a seperate function, wrapping the call to
some_api_function(), or (if the buffer is not modified) use a function which
returns a pointer to the first non-'0' character. Or (if appropriate) use
"if (elem->tag == x || elem->tag == y) ..."

Alex

 

[Michael Mair]

In the course of examining the code for an Internet-connected
authentication server, I came across the following code (twice in one
function with different constants in the "if") in a file of some 2000
non-comment lines written by a "C" expert:

if( elem->tag == SOME_MANIFEST_CONSTANT ) /* Alt */
{
char *tempValue = malloc(32);
strcpy(tempValue, elem->value);

- there is no test to check whether malloc() succeeded
- 32 is a magic number which sooner or later _will_ break the code;
use the same symbolic constant used for obtaining the storage
for elem->value or, if elem->value is a string (as indicated by
the use of strcpy()), malloc(strlen(elem->value) + 1)
- only with large enough buffer pointed to by tempValue,
we can be sure that there is no risk of buffer overrun by
calling strcpy() (using strlen(elem->value) + 1 guarantees
that).
strcpy() and even the safer variant strncpy() are slightly flawed
and have to be handled with care. If this part is not time critical
and you have a standard library which already has snprintf() (a C99
standard library function), you may consider using this instead and
just check the return value.

while( tempValue[0] == '0' )
{
tempValue++;
}
some_api_funtion(tempValue); /* Alt */

free(tempValue);
Otherwise you have a memory leak.

}

(The comments are mine and denote lines that have been altered
for brevity or clarity)

I wonder if the denizens of comp.lang.c would care to comment?

Ask your '"C" expert' whether he really considers the original
version good code. If he does, then there goes the expert rank.
If he does not and this is a crappy module whipped up in short
time to meet some deadlines, you should reconsider your software
production process.


Cheers
Michael

 

[Chris Torek]

In the course of examining the code for an Internet-connected
authentication server, I came across the following code ...

if( elem->tag == SOME_MANIFEST_CONSTANT ) /* Alt */
{
char *tempValue = malloc(32);
strcpy(tempValue, elem->value);
while( tempValue[0] == '0' )
{
tempValue++;
}
some_api_funtion(tempValue); /* Alt */
}

As others have said, it is certainly peculiar code. Without
knowing anything more about it, I would probably have written
something more along these lines:

if (elem->tag == SOME_MANIFEST_CONSTANT) {
/*
* Find the part beginning with a '0' character, which
* is guaranteed to exist, then duplicate that and the
* remainder of the string to give room for the api_zog()
* function to modify.
*
* This comment would make much more sense if we had any
* idea *why* we are finding the first '0' character and
* copying the remainder.
*/
char *p = strchr(elem->value, '0');

if (p == NULL)
panic("gronkelflatz(): elem->value (%s) missing required '0'",
elem->value);
p = xstrdup(p); /* malloc+strcpy, with panic if NULL */
api_zog(p);
free(p);
}

 

[websnarf]

In the course of examining the code for an Internet-connected
authentication server, I came across the following code (twice in one
function with different constants in the "if") in a file of some 2000
non-comment lines written by a "C" expert:

if( elem->tag == SOME_MANIFEST_CONSTANT ) /* Alt */
{
char *tempValue = malloc(32);
strcpy(tempValue, elem->value);

What makes you so sure 32 characters is enough to hold the length of
elem->value? Usually, in place of explicit constants such as "32" you
use a #define which makes your meaning clear, such as MAX_VALUE_LEN,
which you then use in place of 32 above, and semantically as required
in other places in your code.

As others have pointed out, malloc can return with NULL, so technically
you should handle that case.

while( tempValue[0] == '0' )
{
tempValue++;
}

It appears as though what you are trying to do here is to skip a '0'
(possibly repeated) prefix the value of your string. But what if the
string contents itself is nothing but '0's? Is it really your
intention to strip them all away? Assuming that you were trying to
text encode a numeric value of some kind, the number "0" would be
reduced to the empty string here.

some_api_function(tempValue); /* Alt */
}

Ok, as others have pointed out you have a memory leak here, because you
forgot to use free() on what was given to you from malloc. You cannot
simply put free(tempValue) before the } because you've *lost* the
original pointer due to your destructive operation of incrementing it,
as a means of trying to "get rid of leading 0s". This may not be a big
deal if you are using something called the Boehm garbage collector
tool, however, you did not make this clear.

Let's try for a rewrite:

if( elem->tag == SOME_MANIFEST_CONSTANT ) /* Alt */
{
char *s, *t = elem->value;
int len;

while (t[0] == '0' && t[1] !='\0') t++;
len = strlen (t) + 1;
s = malloc (len * sizeof (char));
if (s) {
memcpy (s, t, len);
some_api_function (s);
free (s); /* Unless the function above frees it */
} else {
/* Do something about the failure of malloc */
}
}

So what we've done is we've scanned for the prefixed '0's in the
original string before its copied, as we are allowing for a single "0"
digit without truncating it down to "". Then we've allocated exactly
as much space as it required by the string we are copying. We've
checked for a possible failure of malloc. And finally we free the
memory for the copied string ourself (or we could remove that free()
line and require that the function "some_api_function" do the freeing,
or some other mechanism such as the Boehm garbage collector.)