Anything wrong with this function?

[Tinkertim]

Hello,

I've written a function similar to strdup(), except that realloc() is
used and the # of characters written is returned instead of returning
a cast value of memcpy().

I made this to use in nested loops in lieu of strdup(), to avoid
freeing temporary vars until the end.

Use would be like this:

char *tmp;
.... loop ...
if (-1 == (redup(&tmp, string)))
(bail and complain)
.... end loop ...

if (tmp)
free(tmp);

Beyond failing to set errno, can anyone find anything wrong with this:

int redup(char **s1, const char *s2)
{
size_t len;

if (NULL == s2)
return -1;

len = strlen(s2) + 1;

if (NULL == (*s1 = realloc(*s1, len)))
return -1;

memset(*s1, 0, sizeof(*s1));
memcpy(*s1, s2, len);

return (int) len;
}

Thanks in advance! I'm putting together a small snippet collection for
trimming, tokenizing and managing strings .. I hope to enlist a few
other sets of eyes before I release it into the wild.

Cheers,
--Tim

 

[Tinkertim]

Beyond failing to set errno, can anyone find anything wrong with this:

.... and not typing it as size_t instdead of int, sorry pasted from the
wrong buffer.

Cheers,
--Tim

 

[Nick Keighley]

I've written a function similar to strdup(), except that realloc() is
used and the # of characters written is returned instead of returning
a cast value of memcpy().

I made this to use in nested loops in lieu of strdup(), to avoid
freeing temporary vars until the end.

Use would be like this:

char *tmp;
... loop ...

you need to initialise tmp. Presumably to NULL

if (-1 == (redup(&tmp, string)))

I know why people put the test this way round but its
not a style I like.

   (bail and complain)
... end loop ...

if (tmp)
  free(tmp);

free(NULL) is well defined so you don't need the test

Beyond failing to set errno, can anyone find anything wrong with this:

setting errno isn't a requirement

int redup(char **s1, const char *s2)
{
        size_t len;

        if (NULL == s2)
                return -1;

        len = strlen(s2) + 1;

        if (NULL == (*s1 = realloc(*s1, len)))
                return -1;

if realloc() fails you've just nuked the value
of s1. Its better to use a temporary ptr. I know
your program is going to "bail" but a general purpose
library function shouldn't force this upon you
(unless its aboty() or something...)

airbus.c line 666: program bailed due to realloc failure

        memset(*s1, 0, sizeof(*s1));

you don't really need the memset()...

        memcpy(*s1, s2, len);

....if you terminate the string

*s1[len] = '\0';

        return (int) len;

why the cast? Or rather why does the function return an int?

}

Thanks in advance! I'm putting together a small snippet collection for
trimming, tokenizing and managing strings .. I hope to enlist a few
other sets of eyes before I release it into the wild.

--
Nick Keighley

A designer knows he has achieved perfection
not when there is nothing left to add,
but when there is nothing left to take away.
Antoine de Saint-Exupéry

 

[Tinkertim]

you need to initialise tmp. Presumably to NULL

Yes, that was an off the hip usage example.

I know why people put the test this way round but its
not a style I like.

It grew to be a habit so I could easily see what I was comparing vs
assigning. I should probably do that a bit more selectively (when
really needed) .. others have said the same.

free(NULL) is well defined so you don't need the test

Its being used on an OS which is writing userspace libc from scratch,
but the snippet should probably cater to everyone else. Thanks, noted.

setting errno isn't a requirement

It seemed a nice creature comfort to do so, enabling the calling
function to convey something more useful to the user and decide better
on what to do next.

if realloc() fails you've just nuked the value
of s1. Its better to use a temporary ptr. I know
your program is going to "bail" but a general purpose
library function shouldn't force this upon you
(unless its aboty() or something...)

Yes, if realloc() fails I should just hand the string back as I found
it and return -1. This was why I was thinking of setting errno since
multiple things could go wrong.

airbus.c line 666: program bailed due to realloc failure

Point noted. Its so funny, NASA enforces strict rules which forbid
dynamic allocation. Perhaps that makes Mars landers safer than
airplanes? Who knows.

memset(*s1, 0, sizeof(*s1));

you don't really need the memset()...

memcpy(*s1, s2, len);

...if you terminate the string

*s1[len] = '\0';

Again, correct. Noted and thanks. I'm glad that I posted this.

why the cast? Or rather why does the function return an int?

In my follow up I corrected that, the function should be size_t and
its return value should not be cast .. else very big strings could
cause undefined behavior.

Thanks again Nick!

Cheers,
--Tim

 

[Ben Bacarisse]

Be careful here. The memset is wrong rather than "not needed". It
does not terminate the string and may invoke undefined behaviour if
the object at *s1 is smaller than sizeof(*s1).

memcpy(*s1, s2, len);

...if you terminate the string

*s1[len] = '\0';

Again, correct. Noted and thanks. I'm glad that I posted this.

Also, the assignment *s1[len] = '\0'; is wrong in two ways. It writes
outside of the allocated space and it is not needed since the memcpy
has already copied a null. Part of the problem is that Nick has been
taken in by your name -- len is not the length of the string, you have
already added one for the null. I'd change it to 'size'.

 

[James Kuyper]

Tinkertim wrote:
....

Point noted. Its so funny, NASA enforces strict rules which forbid
dynamic allocation.

Not as a universal rule. I work for a contractor on a NASA project; our
project's software allocates memory dynamically in many places. However,
our software is strictly for analyzing data that has came down from a
satellite; the rules for flight operations software might be more
restrictive.

 

[Keith Thompson]

On Oct 2, 3:43 pm, Nick Keighley <[email protected]>
wrote: [...]

I know why people put the test this way round but its
not a style I like.

It grew to be a habit so I could easily see what I was comparing vs
assigning. I should probably do that a bit more selectively (when
really needed) .. others have said the same.

[...]

When is it "really needed"? If you're paying enough attention to know
when it's necessary and when it isn't, I'd say you're paying enough
attention so that you don't really need to reverse the arguments in
the first place.

 

[Barry Schwarz]

Hello,

I've written a function similar to strdup(), except that realloc() is
used and the # of characters written is returned instead of returning
a cast value of memcpy().

I made this to use in nested loops in lieu of strdup(), to avoid
freeing temporary vars until the end.

Use would be like this:

char *tmp;
... loop ...
if (-1 == (redup(&tmp, string)))
(bail and complain)
... end loop ...

if (tmp)
free(tmp);

Beyond failing to set errno, can anyone find anything wrong with this:

int redup(char **s1, const char *s2)
{
size_t len;

if (NULL == s2)
return -1;

len = strlen(s2) + 1;

if (NULL == (*s1 = realloc(*s1, len)))

If realloc returns NULL, you have lost the original value of *s1. If
that value was not NULL you have created a memory leak.

return -1;

memset(*s1, 0, sizeof(*s1));

First off, this is useless since the next statement completely
replaces the entire block *s1 points to.

Secondly, it doesn't initialize the correct quantity of bytes. s1 is
a char**; therefore *s1 is a char*. sizeof *s1 will probably be
either 4 or 8. If this value is greater than len, you invoke
undefined behavior.

 

[Flash Gordon]

Tinkertim wrote, On 02/10/08 08:04:

if (NULL == (*s1 = realloc(*s1, len)))
return -1;

memset(*s1, 0, sizeof(*s1));
memcpy(*s1, s2, len);

return (int) len;
}

Thanks in advance! I'm putting together a small snippet collection for
trimming, tokenizing and managing strings .. I hope to enlist a few
other sets of eyes before I release it into the wild.

Something I've not noticed anyone else mentioning is that the above
could be horribly inefficient, possibly more inefficient that simply
freeing the old buffer and allocating a new one. The reason is that
realloc is required to keep data, so if you have a 1024 byte string and
realloc to a larger size buffer causing realloc to move the buffer it
will have to copy the entirity of the original string which you don't
actually need!

 

[Lassie]

"Nick Keighley" <[email protected]> schreef in bericht

I know why people put the test this way round but its
not a style I like.

yeah, the style annoys me. Whenever I see a line like that I hickup in
reading the code.