Anything wrong with this function?

**Tinkertim** · Oct 2 '08, 07:15 AM

Re: Anything wrong with this function?

On Oct 2, 3:04 pm, Tinkertim <tinker...@gmai l.comwrote:

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** · Oct 2 '08, 07:45 AM

Re: Anything wrong with this function?

On 2 Oct, 08:04, Tinkertim <tinker...@gmai l.comwrote:

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** · Oct 2 '08, 08:45 AM

Re: Anything wrong with this function?

On Oct 2, 3:43 pm, Nick Keighley <nick_keighley_ nos...@hotmail. com>
wrote:

you need to initialise tmp. Presumably to NULL

Yes, that was an off the hip usage example.

>

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

>
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.

>

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

>

if (tmp)
free(tmp);

>
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.

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

>
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.

return (int) len;

>
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** · Oct 2 '08, 10:45 AM

Re: Anything wrong with this function?

Tinkertim <tinkertim@gmai l.comwrites:

On Oct 2, 3:43 pm, Nick Keighley <nick_keighley_ nos...@hotmail. com>
wrote:

>you need to initialise tmp. Presumably to NULL

<snip>

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

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

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'.

--
Ben.

**James Kuyper** · Oct 2 '08, 11:55 AM

Re: Anything wrong with this function?

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** · Oct 2 '08, 03:15 PM

Re: Anything wrong with this function?

Tinkertim <tinkertim@gmai l.comwrites:

On Oct 2, 3:43 pm, Nick Keighley <nick_keighley_ nos...@hotmail. com>
wrote:

[...]

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

>>
>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.

--
Keith Thompson (The_Other_Keit h) kst-u@mib.org <http://www.ghoti.net/~kst>
Nokia
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"

**blargg** · Oct 2 '08, 11:45 PM

Re: Anything wrong with this function?

In article <87abdncm3m.fsf @bsb.me.uk>, Ben Bacarisse
<ben.usenet@bsb .me.ukwrote:

Tinkertim <tinkertim@gmai l.comwrites:
>

On Oct 2, 3:43 pm, Nick Keighley <nick_keighley_ nos...@hotmail. com>
wrote:

you need to initialise tmp. Presumably to NULL

<snip>

memset(*s1, 0, sizeof(*s1));
>
you don't really need the memset()...

>
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).

Yep, virtually any memset call of the pattern (where n is omitted if it's 1)

memset( ptr, fill, n * sizeof ptr )

is wrong, since you're asking it to fill the bytes pointed TO, but giving
it the size of the pointer itself. In almost all cases, it should be

memset( ptr, fill, n * sizeof *ptr )

**Barry Schwarz** · Oct 3 '08, 04:55 AM

Re: Anything wrong with this function?

On Thu, 2 Oct 2008 00:04:25 -0700 (PDT), Tinkertim
<tinkertim@gmai l.comwrote:

>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.

> memcpy(*s1, s2, len);
>
> return (int) len;
>}
>

--
Remove del for email

**Flash Gordon** · Oct 3 '08, 06:05 PM

Re: Anything wrong with this function?

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

<snip>

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!
--
Flash Gordon
If spamming me sent it to smap@spam.cause way.com
If emailing me use my reply-to address
See the comp.lang.c Wiki hosted by me at http://clc-wiki.net/

**Lassie** · Oct 5 '08, 07:05 AM

Re: Anything wrong with this function?

"Nick Keighley" <nick_keighley_ nospam@hotmail. comschreef in bericht
news:430d8068-db5f-4251-ac45-6160dececcef@h2 g2000hsg.google groups.com...
On 2 Oct, 08:04, Tinkertim <tinker...@gmai l.comwrote:

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

>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.

**Richard Heathfield** · Oct 5 '08, 07:25 AM

Re: Anything wrong with this function?

Lassie said:

>
"Nick Keighley" <nick_keighley_ nospam@hotmail. comschreef in bericht
news:430d8068-db5f-4251-ac45-6160dececcef@h2 g2000hsg.google groups.com...
On 2 Oct, 08:04, Tinkertim <tinker...@gmai l.comwrote:

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

>

>>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.

And vice versa: although in *this* case I'm easy either way (because it's a
function call returning a mere int), my reaction in the general case:
(i.e. x == k where x is an object) is invariably "whoa, object on left,
object on left, check for =/== bug", so in comparisons for equality I put
the constant on the left where possible (i.e. if there is one), to
minimise my discomfiture.

I would not, however, expect you to change your style for my sake. And
neither will I change mine for your sake.

--
Richard Heathfield <http://www.cpax.org.uk >
Email: -http://www. +rjh@
Google users: <http://www.cpax.org.uk/prg/writings/googly.php>
"Usenet is a strange place" - dmr 29 July 1999

Anything wrong with this function?

Anything wrong with this function?

Comment

Comment

Comment

Comment

Comment

Comment

Comment

Comment

Comment

Comment

Comment