gets() - dangerous?

[Markus Becker]

This has the overhead of copying (CAPACITY_OF_DST - 1 - strlen(src)) zero

I had expected this and thought of an appropriate answer,
but in the meantime I have learned a few things. Anyway,
the answer to this argument is that otherwise you always
have the overhead of calling strlen(src), which shouldn't
be much different.

And I have slept a good time. Up to my previous posting
I've had about 4 hours of sleep this whole year.

It also silently truncates strings that are oversize: this semantic is not

You can check that with a (strlen(dst)<CAP_OF_DST-1) afterwards.

...which avoids the redundant copying overhead. Often the call to

but does not copy any data. This too might be sub-optimal.

strlen(src) is required for a prior operation and its result is at hand
anyhow. (the comparison shouldn't include an equality test btw)

Right, but I was very tired.

Well, if truncating the string is appropriate semantics, then fine.

I see we agree on the fact that it mostly depends, but there are
several possibilities to handle strcpy et al. without too much
hassle.

Another possibility that may be appropriate is to (re)assign a buffer
large enough to hold the string. At least this code snippet gives you the
opportunity to take that action.

See my answer to targeur fou. Thanks to you, too!

Markus

 

[David Resnick]

Yes, and there are cases where it is just plain silly to report
a 'buffer overflow' which just has not happend yet and can be
worked around. Another solution could be to realloc the dst
and try again, this time with strcpy because I ... oh, my,
it's beginning to get clear. Sure, there are hundreds of
way to use strcpy correctly, one of them being:

char *dst = malloc(strlen(src));
if (dst) strcpy(dst,src);

Good way to use strcpy incorrectly. Above has fencepost error.
(needs to malloc length + 1 for terminating '\0' character), it will
overwrite the bounds of the malloced region by 1.

-David

 

[Markus Becker]

Good way to use strcpy incorrectly. Above has fencepost error.

Yep, you got me.

(needs to malloc length + 1 for terminating '\0' character), it will
overwrite the bounds of the malloced region by 1.

If I were little Kenny MacTroll, my answer would be:

"I did not expect _you_ to find my nasty little mistake
that I put into my snippet."

I really did not think long enough because I already
was further ahead in my thoughts.

Thanks for pointing this out or it could have made
into my set of snippets that you use thoughtlessly,
because you 'know' they're correct.

Jungejunge.

markus

 

[Richard Heathfield]

David Resnick said:

Good way to use strcpy incorrectly. Above has fencepost error.

No, it has an off-by-one error. A fencepost error is indeed an off-by-one
error, but not all off-by-one errors are fencepost errors.

 

[Flash Gordon]

How? HOW!!!1

Any time when you know the destination buffer is at least as large as
the source buffer you can use strcpy without worrying about the length
of the string.

Not absolutely, but if I want to work on the data even if my destination
buffer is too small.

Why is your destination buffer shorter than your source buffer? ;-)

Yes, and there are cases where it is just plain silly to report
a 'buffer overflow' which just has not happend yet and can be
worked around. Another solution could be to realloc the dst
and try again, this time with strcpy because I ... oh, my,
it's beginning to get clear. Sure, there are hundreds of
way to use strcpy correctly, one of them being:
<snip>

Indeed.

At this point most of my post has been rendered needless but
since I typed it anyway, I'll leeave my 'argumentation' there
for your amusement.

 

[Michael Wojcik]

It's written to avoid overflows, not to warn about them.

Which, in many cases, is nearly as bad. Silent truncation of data
is also a security hole.

The damage has already been done by this point:

No it hasn't. That's snprintf, not sprintf.

That said, as I've argued in another thread, I think snprintf is a
poor choice for this particular task. (It's unnecessarily heavy for
the job; it's variadic, so the fourth parameter isn't type-safe; it
returns int rather than size_t; the format string is an unnecesary
opportunity for error; it's not in C90 and there are prominent
noncompliant implementations in existence.)

IMO, there is no standard function which is entirely satisfactory
for copying strings. Fortunately, it's not difficult to write one
(or rather a few, since there are an assortment of string-copying
tasks with different requirements), and many people have.

--
Michael Wojcik (e-mail address removed)

Q: What is the derivation and meaning of the name Erwin?
A: It is English from the Anglo-Saxon and means Tariff Act of 1909.
-- Columbus (Ohio) Citizen

 

[Chuck F.]

.... snip ...

IMO, there is no standard function which is entirely
satisfactory for copying strings. Fortunately, it's not
difficult to write one (or rather a few, since there are an
assortment of string-copying tasks with different requirements),
and many people have.

Once more, into the breech rode the six-hundred !!

A very good solution has been proposed by the BSD group. It is
called strlcpy and strlcat. I have posted a complete
implementation (in purely standard portable C) and links to the
original proposal at:

<http://cbfalconer.home.att.net/download/strlcpy.zip>

--
"If you want to post a followup via groups.google.com, don't use
the broken "Reply" link at the bottom of the article. Click on
"show options" at the top of the article, then click on the
"Reply" at the bottom of the article headers." - Keith Thompson
More details at: <http://cfaj.freeshell.org/google/>

 

[Netocrat]

No it hasn't. That's snprintf, not sprintf.

True; my reading and post were too hasty - I withdraw my comment that
the code posted by DES is in itself exploitable.

 

[Richard Bos]

Agreed.

To avoid the dangers of the two points above, I would use something
like this in my programs:

lngSrc = strlen(p);
lngDst = (lngSrc > MAXCH_BUF1 ) ? MAXCH_BUF1 : lngSrc ;
strncpy (buffer1, p, lngDst+1);

lngSrc = strlen(buffer1);
lngDst = (lngSrc > MAXCH_BUF2 ) ? MAXCH_BUF2 : lngSrc ;
strncpy (buffer2, buffer1, lngDst+1);

Why bother working around the broken design of strncpy(), when you can
get the right behaviour by replacing


fiddle_with_num_until_it_is_efficient_enough(possibly_costly_strlen());
strncpy(dest, scr, num);

fiddle_with_dest_until_it_is_a_conforming_string(possibly_costly_strlen());

with

*dest='\0';
strncat(dest, scr, num-1);

and have the correct, desired behaviour with a single, simple assignment
and one equally simple function call?

I don't really understand the religious war between strcpy/strncpy pros
and cons since it's not really difficult for an experienced programmer
to write a safe program with one or the other.

There's no religious war. strncpy() is simply not designed to work with
C strings.

Richard

 

[Richard Bos]

You're right, there's no chance to use gets in a way that
makes it safe. But strcpy can be used in an unsafe way,

So can printf(). So can strncpy() - in fact, it usually will. So can
realloc(). If you can't _think_ before you code, C is not the language
for you - there's always Logo. The problem with gets() is that, unlike
all other functions in C, it won't allow you to think.

When I read the man pages regarding gets and strcpy,
they mention BUGS and dangerous and such in the case
of gets -> this is clearly dangerous.
In the case of strcpy I read "the buffer that dst points
to has to be big enough and the too strings must not
overlap" -> this _can_ be made safe, but it is tedious(sp?)

If your man page considers this a bug, it is broken.

So what do you mean by 'if used correctly' regarding both
strcpy and gets? IMHO the only 'correct' use of gets
it to NOT use gets. Never.
True.

There are several possibilities
to use strcpy correctly, but all involve calls to other
functions, an "if ()" and

No, some will involve knowing in advance how long your strings are,
because you read them using fgets(). If you know that you have two input
strings of N chars and an output buffer of 2N chars, you never need
worry about

strcpy(dest, src1);
strcat(dest, src2);

This is not tedious and does not involve strlen(); it is called planning
ahead, a skill all programmers should have but astonishingly few do.

an "else strncpy(...)",

Never at all. strncpy() is not suitable for use on C strings unless
you're a masochist.

Always use "strncpy(dst,src,CAPACITY_OF_DST-1);"

as compared to this:

if (strlen(src)<=CAPACITY_OF_DST) strcpy(dst,src);
else /* do something accordingly, which will be: */
strncpy(...);

This would not get past a desk-check by me. It is daft.

Richard

 

[Richard Bos]

Ok, but then, consequentially, every function concerning
C-strings (null-terminated char arrays) should be considered
to have traps. And many more. What about system()?
Just kidding ...

Says it all, really. system() _does_ have traps, and the wise programmer
is aware of them. The unwise programmer calls system("cls"); and is
surprised that his program appears to print nonsense on Unix systems.

The two strings must not overlap too with strncpy().

I know[1], violating this constraint might produce garbled
data (could be dangerous, too, depending on what you
do with the data afterwards) but will definitely _not_
produce a buffer overflow in the sense that anything
outside of src or dst gets overwritten.

You do not know this. From the Standard:

# If copying takes place between objects that overlap, the behavior is
# undefined.

Richard

 

[Daniel Rudy]

At about the time of 1/3/2006 5:59 AM, Christian Bau stated the following:

The more dangerous problem with strncpy is: If the destination buffer is
not large enough, what it copies _is not a valid C string_!!! It doesn't
append a trailing zero! If you use strncpy to copy into a 10 byte
buffer, and the source string is too long, it copies 10 bytes instead of
copying 9 bytes and a trailing zero, which at least would have given you
a valid C string.

I guess it is time to write your own function that does what it should
do: strcpy if the result fits, copy a valid C string by dropping
trailing characters if the result doesn't fit. You still have to be
careful, but at least you won't get a buffer overflow and undefined
behavior.

There already is a function that does that: strlcpy and strlcat. The
performance is much higher for strlcpy than for strncpy.


--
Daniel Rudy

Email address has been base64 encoded to reduce spam
Decode email address using b64decode or uudecode -m

Why geeks like computers: look chat date touch grep make unzip
strip view finger mount fcsk more fcsk yes spray umount sleep

 

[Chuck F.]

Christian Bau stated the following:

There already is a function that does that: strlcpy and strlcat.
The performance is much higher for strlcpy than for strncpy.

You neglected to point out that these are non-standard functions.
However purely standard code for them, needing nothing more than
compilation on any system, is available at:

<http://cbfalconer.home.att.net/download/strlcpy.zip>

--
"If you want to post a followup via groups.google.com, don't use
the broken "Reply" link at the bottom of the article. Click on
"show options" at the top of the article, then click on the
"Reply" at the bottom of the article headers." - Keith Thompson
More details at: <http://cfaj.freeshell.org/google/>

 

[Daniel Rudy]

At about the time of 1/3/2006 1:27 AM, Markus Becker stated the following:

It's more easy to enforce a coding rule regarding string copying
that reads like this:

Always use "strncpy(dst,src,CAPACITY_OF_DST-1);"

as compared to this:

if (strlen(src)<=CAPACITY_OF_DST) strcpy(dst,src);
else /* do something accordingly, which will be: */
strncpy(...);

So for 'me', strncpy is safer than strcpy and if I try to use
strcpy correctly I will need to program a call to strncpy anyway
if the length of src gets (oh my, this word again) greater than
the capacity of the buffer pointed to by dst.

What would be the 'proven' way of using strcpy in a safe
way without needing strncpy as a fallback? I wouldn't be
surprised if I had overlooked something obvious. Sometimes
I cannot see the wood for the trees, you know. Especially
after a long night like this...

Markus

Here's something that's really fast:

#include <strings.h>
#include <string.h>

int strscpy(char *dest, const char *src, int dest_size)
{
int str_size; /* size of string to be copied */
int copy_size; /* number of characters to copy */

/* get src string size */
str_size = strlen(src);
/* determine how much to copy */
/* str_size + 1 is based on strlen not including the terminating
null in the size of the string. remove it if your
implementation does */
copy_size = str_size + 1 < dest_size - 1 ? str_size : dest_size - 2;
/* copy */
bcopy(src, dest, copy_size);
/* set last character to null */
dest[dest_size - 1] = 0x00;
/* return to caller with number of characters copied */
return(copy_size);
}

The benifit of this is that all you do is give it the size of the
destination, and because it is using bcopy, it will handle strings that
overlap in memory. Furthermore, it has significant performance because
it only copies what needs to be copied, and as a precationary action it
sets the very last byte in dest to null.


if (strscpy(dest, src, sizeof(dest)) < strlen(src))
{
/* take approperiate action here */
}


--
Daniel Rudy

Email address has been base64 encoded to reduce spam
Decode email address using b64decode or uudecode -m

Why geeks like computers: look chat date touch grep make unzip
strip view finger mount fcsk more fcsk yes spray umount sleep

 

[usenet]

Here's something that's really fast:

#include <strings.h>
#include <string.h>

int strscpy(char *dest, const char *src, int dest_size)
{
int str_size; /* size of string to be copied */
int copy_size; /* number of characters to copy */

/* get src string size */
str_size = strlen(src);
/* determine how much to copy */
/* str_size + 1 is based on strlen not including the terminating
null in the size of the string. remove it if your
implementation does */
copy_size = str_size + 1 < dest_size - 1 ? str_size : dest_size - 2;
/* copy */
bcopy(src, dest, copy_size);
/* set last character to null */
dest[dest_size - 1] = 0x00;
/* return to caller with number of characters copied */
return(copy_size);
}

The benifit of this is that all you do is give it the size of the
destination, and because it is using bcopy, it will handle strings that
overlap in memory.

It may be fast, but my system does not provide a bcopy() function...

 

[Flash Gordon]

At about the time of 1/3/2006 1:27 AM, Markus Becker stated the following:


Here's something that's really fast:

Yes, it take absolutely no run time at all because it fails to compile.

#include <strings.h>

No such header in standard C, and it does not exist on all of the
implementations I use including one very popular platform.

#include <string.h>

int strscpy(char *dest, const char *src, int dest_size)

What if the buffer is larger than INT_MAX? Wouldn't size_t be more
appropriate for the size?

int str_size; /* size of string to be copied */
int copy_size; /* number of characters to copy */

See comments above.

/* get src string size */
str_size = strlen(src);

Oh dear, I passed in a string longer than INT_MAX.

/* determine how much to copy */
/* str_size + 1 is based on strlen not including the terminating
null in the size of the string. remove it if your
implementation does */

The above comment is rather daft. By *definition* strlen does not
include the null termination in the length of the string. Therefore you
would need to be using something other than a C compiler to not need the +1.

copy_size = str_size + 1 < dest_size - 1 ? str_size : dest_size - 2;
/* copy */
bcopy(src, dest, copy_size);

Non-standard function that does not exist on at least one very popular
platform. Why don't you use memmove which *does* exist because it is
part of the C standard?

/* set last character to null */
dest[dest_size - 1] = 0x00;
/* return to caller with number of characters copied */
return(copy_size);
}

The benifit of this is that all you do is give it the size of the
destination,

However it will fail if either the size of the source string (including
null termination) or the size of the destination buffer is larger than
can be represented in a string. Rather inconsistent with the functions
in the standard C library.

> and because it is using bcopy, it will handle strings that
overlap in memory.

No, because it uses bcopy it is not portable and won't even compile on
some significant platforms.

> Furthermore, it has significant performance because
it only copies what needs to be copied, and as a precationary action it
sets the very last byte in dest to null.

If fixed so that it actually worked those would be good points.

if (strscpy(dest, src, sizeof(dest)) < strlen(src))
{
/* take approperiate action here */
}

In future, please post standard C answers not implementation specifics,
especially when there is a simple standard C way of doing the same
thing. We only deal with standard C here, not the BSD extensions, POSIX
extensions or Windows extensions.

 

[Chuck F.]

At about the time of 1/3/2006 1:27 AM, Markus Becker stated the following:
.... snip ...

if (strscpy(dest, src, sizeof(dest)) < strlen(src))
{
/* take approperiate action here */
}

You have re-invented strlcpy, without some of the provisions
specified for its action. For details of this see:

<http://cbfalconer.home.att.net/download/strlcpy.zip>

At any rate, here is the heart code of my implementation. Notice
that it doesn't use the library at all, and thus is suitable for
embedded applications. Since it has to scan the length of the
source string it combines that with the actual transfer, for
noticeable efficiency improvement.

size_t strlcpy(char *dst, const char *src, size_t sz)
{
const char *start = src;

if (src && sz--) {
while ((*dst++ = *src))
if (sz--) src++;
else {
*(--dst) = '\0';
break;
}
}
if (src) {
while (*src++) continue;
return src - start - 1;
}
else if (sz) *dst = '\0';
return 0;
} /* strlcpy */

/* ---------------------- */

size_t strlcat(char *dst, const char *src, size_t sz)
{
char *start = dst;

while (*dst++) /* assumes sz >= strlen(dst) */
if (sz) sz--; /* i.e. well formed string */
dst--;
return dst - start + strlcpy(dst, src, sz);
} /* strlcat */

--
"If you want to post a followup via groups.google.com, don't use
the broken "Reply" link at the bottom of the article. Click on
"show options" at the top of the article, then click on the
"Reply" at the bottom of the article headers." - Keith Thompson
More details at: <http://cfaj.freeshell.org/google/>

 

[Kenny McCormack]

There's no religious war. strncpy() is simply not designed to work with
C strings.

"There's no religious war. Just a difference of opinion..."

(RB, speaking of the Crusades)

 

[Christian Bau]

At about the time of 1/3/2006 1:27 AM, Markus Becker stated the following:

It's more easy to enforce a coding rule regarding string copying
that reads like this:

Always use "strncpy(dst,src,CAPACITY_OF_DST-1);"

as compared to this:

if (strlen(src)<=CAPACITY_OF_DST) strcpy(dst,src);
else /* do something accordingly, which will be: */
strncpy(...);

So for 'me', strncpy is safer than strcpy and if I try to use
strcpy correctly I will need to program a call to strncpy anyway
if the length of src gets (oh my, this word again) greater than
the capacity of the buffer pointed to by dst.

What would be the 'proven' way of using strcpy in a safe
way without needing strncpy as a fallback? I wouldn't be
surprised if I had overlooked something obvious. Sometimes
I cannot see the wood for the trees, you know. Especially
after a long night like this...

Markus

Here's something that's really fast:

#include <strings.h>
#include <string.h>

int strscpy(char *dest, const char *src, int dest_size)
{
int str_size; /* size of string to be copied */
int copy_size; /* number of characters to copy */

/* get src string size */
str_size = strlen(src);
/* determine how much to copy */
/* str_size + 1 is based on strlen not including the terminating
null in the size of the string. remove it if your
implementation does */
copy_size = str_size + 1 < dest_size - 1 ? str_size : dest_size - 2;
/* copy */
bcopy(src, dest, copy_size);
/* set last character to null */
dest[dest_size - 1] = 0x00;
/* return to caller with number of characters copied */
return(copy_size);
}

What is bcopy? And what bright spark defined a copy function that has
its argument the other way round than the standard memcpy?

 

[Jordan Abel]

You neglected to point out that these are non-standard functions.
However purely standard code for them,

Except for the use of reserved identifiers,