8 ms·

Strncpy() is not a “safer” strcpy()

56 points by luu 11y ago

lawl 11y ago
tl;dr: strncpy() doesn't null terminate, but doesn't mention strlcpy() http://www.openbsd.org/cgi-bin/man.cgi?query=strlcpy http://www.openbsd.org/cgi-bin/man.cgi?query=strlcpy
- masklinn 11y ago
  Because, much like reallocarray, strlcpy is not part of the C standard library. The correct solution, assuming you can't just import libbsd or include its functions, is probably to use snprintf. It goes guarantee null-termination of the target. So strncpy(dst, src, size_dst); becomes snprintf(dst, size_dst, "%s", src); Checking for partial copy is not ideal though, snprintf returns the number of bytes it would have copied had the destination buffer been infinite excluding the final NUL byte. So a copy is only complete if `ret < size_dst`. snprintf is not available in C89 though...
  - rkangel 11y ago
    Or just to add an implementation of strlcpy in a util module in your codebase. e.g.: http://mirror.fsf.org/pmon2000/3.x/src/sdk/libc/string/strlcpy.c http://mirror.fsf.org/pmon2000/3.x/src/sdk/libc/string/strlc...
    - masklinn 11y ago
      Hence > assuming you can't just import libbsd or include its functions
      - lmm 11y ago
        Under what circumstances would you ever be unable to copy such a small and liberally-licensed file into your source?
        nitrogen 11y ago
        If, for whatever reason, you don't want to have anyone else's copyright on your product, or don't want to keep track of which licenses have to be displayed where.
  - jjnoakes 11y ago
    snprintf is also slower. It has to parse the format string. Just use strncpy and NUL-terminate the buffer. It's not hard. Write a two line function to do it for you, and use that function in place of strncpy.
    - _kst_ 11y ago
      dest[0] = '\0'; strncat(dest, src, size);
      - jjnoakes 11y ago
        Sure. Or: strncpy(dest, src, size); dest[size - 1] = '\0';
        nitrogen 11y ago
        The fact that strncpy zeros the buffer may make it slower.
        jjnoakes 11y ago
        Sure, by a couple of cycles. Unless your size is huge. In which case, perhaps you should try to avoid copying strings in the first place.
    - masklinn 11y ago
      > Just use strncpy and NUL-terminate the buffer. It's not hard. Just as it's not hard to not dereference null pointers, not double-free allocations and a truckload of other mindless drudgery which history shows humans won't reliably get right all the time. Not to mention checking for copy truncation with strncpy is error-prone, you've got to check whether it points to a `\0` before you manually null-terminate it, providing more chances to get it wrong. > snprintf is also slower. It has to parse the format string. How about worrying about that when snprintf actually shows up in profiles and there's no algorithmic way to improve the situation and strncpy is a significant improvement?
- MrBuddyCasino 11y ago
  I'm not a C programmer, but even I knew that strlcpy() and snprintf() would come up in this discussion. How fucked up is that? Is there a resource where these issues are answered once and for all, for the novice C programmer? Also, are there deprecation warnings emitted by the compiler for non-safe C stdlib functions?
  - clarry 11y ago
    On OpenBSD the linker whines about uses of unsafe functions. I don't see it becoming mainstream though, as there are so many people who disagree about what's unsafe.
  - lawl 11y ago
    Not a C programmer either. But I've seen projects banning strcpy. The code was littered with strncpy(a, b, strlen(b)). So I'm not sure how useful these warnings would be.
cremno 11y ago
A similar article: https://randomascii.wordpress.com/2013/04/03/stop-using-strncpy-already/ https://randomascii.wordpress.com/2013/04/03/stop-using-strn... This article is also on GH (including some changes): https://github.com/Keith-S-Thompson/the-flat-trantor-society/blob/master/002-strncpy.md https://github.com/Keith-S-Thompson/the-flat-trantor-society...
StavrosK 11y ago
> That second paragraph means that if the string pointed to by s2 is shorter than n characters, it doesn't just copy n characters and add a terminating null character, which is what you'd expect. But what I would expect is that it would copy len(s2) characters, not n.
- deleted 11y ago
  [deleted]
- patal 11y ago
  That's correct. I think the author meant that.
JulianMorrison 11y ago
C people, isn't it perhaps time to quietly pasture the existing standard library and create a new one, from scratch, that isn't chock full of booby traps in the small print?
- pjmlp 11y ago
  The real problem, how C sees strings, arrays and pointers would still persist. Implicit conversion between vectors and pointers should have never happened. I don't get why people find so hard to write &vector[0] for those cases where a pointer is needed. Also if other languages, for other OS, older than C could do bound checking elision, C not doing it was only saving work for the compiler developer.
  - kazinator 11y ago
    If &array[0] were needed, it wouldn't solve anything. The semantics that you can take the pointer to the first element, and pass it around and treat it as an array, with no meta-data connecting it to the original properties of the array, would still be there. The requirement for a cast would be better, because that's the existing notation for explicitly requesting conversions that are identified as unsafe. I.e. char array[42]; char *p = array; /* error, implicit conversion */ char *q = (char *) array; /* OK */ char *s = &array[0]; /* Sneaky: OK, without a cast */
    - pjmlp 11y ago
      Sure, but I was specially thinking about the issue of passing vectors into functions. In any way, for me the right way of doing systems programming is the way of Mesa, Modula-2 and many others. C was already bad in the 70's.
      - kazinator 11y ago
        I programmed quite a bit in Modula-2 before C. It didn't teach me enough to recognize the silliness in C. I believe that this is an aspect of C popularity that hasn't received a lot of recognition. Programmers were sucked away from cleaner languages into C, because the users of the cleaner languages had neglected to acquire the body of rationalizations for why those languages are that way which could have helped them stick. C looked good in naive ways. "Hey, look at all the things you can easily do with memory and pointers! Wow, look at that slick syntax: you can stuff several increments and an assignment into an expression, and use that as the test of a loop, ... man I'm never writing foo := foo + 4; again!" Modula 2's memory management is basically malloc/free though. While the language could save you from array overruns, it is basically defenseless against leaks and use-after-free bugs, as well as uses of unchecked null pointers under OOM: See: http://www.modula2.org/reference/isomodules/isomodule.php?file=STORAGE.DEF http://www.modula2.org/reference/isomodules/isomodule.php?fi... Look, the allocator works with generic pointers, has an explicit DEALLOCATE, and ALLOCATE returns NIL when there is no memory! Modula 2' standard also has a SYSTEM module which adds C-like things: pointer arithmetic, casting and whatnot. http://www.modula2.org/reference/isomodules/isomodule.php?file=SYSTEM.DEF http://www.modula2.org/reference/isomodules/isomodule.php?fi... Is that just "C envy", or is there perhaps need for those things, know what I mean?
angry_octet 11y ago
For someone being so pedantic it is puzzling that he doesn't address the actual title of his post. No one claims that strncpy is 'safe' they just say 'safer', that is, less likely than normal strcpy to have catastrophic bugs. And I think that claim stands up: make sure your n in strncpy is the length of your buffer, do a strncpy, write a null at the end of the buffer. Voila, all those strcpyies from unexpectedly large strings writing in your heap are gone. No need to constantly strlen source strings to check the length -- which in itself can cause a segfault if they are not null terminated. So yes, it is safer. You would be mad not to use it.
- unwind 11y ago
  Replacing one function call with a function call to a function that does almost the right thing in many cases, but potentially wastes time by pointlessly zero-filling a buffer, and an extra assigment statement is not an improvement. You would be mad to do that.
- kazinator 11y ago
  Starting in C99, you can do this: snprintf(buffer, size, "%s", source); This can be put into a #define macro or inline function. If what you want is a null terminated, truncated copy that fits into a given buffer size, and snprintf is available, you are mad to still use strncpy. Also, strncpy pads the destination with nulls, if there is remaining space. Because of this, the evident purpose of strncpy is to fill fixed-width text fields in "old school" fixed-field database records. Writing the extra nulls is wasteful if the data type in question is nothing but a C string.
  - jedbrown 11y ago
    With fixed-width records, writing the extra nulls may be important for data privacy reasons and in any case, it allows comparison using memcpy(3), diff(1), and checksums.
    - _kst_ 11y ago
      Sure, but then you have to make sure that you always maintain the extra nulls. diff(1) doesn't seem relevant; it operates on text files, which normally don't contain any null characters.
useerup 11y ago
Microsoft has a number of dangerous c functions that are on the Secure Development Lifecycle "banned" list. And yes, strncpy is one of them. Some insights here: https://msdn.microsoft.com/en-us/library/bb288454.aspx https://msdn.microsoft.com/en-us/library/bb288454.aspx strncpy example vulnerability experience linked to from that article "Buffer Overflow in Apache 1.3.xx fixed on Bugtraq - the evils of strncpy and strncat!": http://blogs.msdn.com/b/michael_howard/archive/2004/10/29/249713.aspx http://blogs.msdn.com/b/michael_howard/archive/2004/10/29/24... Source code for MS products are automatically (since SDL) screened for use of the banned functions. You'll have to use the "safer" alternatives.
- deleted 11y ago
  [deleted]
earlz 11y ago
It's a shame that strncpy doesn't have the semantics of OpenBSD's strlcpy, which I think behaves as everyone would expect. http://www.openbsd.org/cgi-bin/man.cgi?query=strlcpy http://www.openbsd.org/cgi-bin/man.cgi?query=strlcpy
gvb 11y ago
There is a very simple, efficient solution: always put a null termination character in the end position of the destination after the strncpy(). If the source is longer than the destination, it will properly terminate the destination. If it isn't longer, it didn't cost (hardly) anything. Unfortunately, it isn't automatic so some times humans will forget to do it. #include <stdio.h> #include <string.h> int main(int argc, char *argv[]) { char *src = "Gobblygook is a long string"; char dest[5]; strncpy(dest, src, sizeof(dest)); dest[sizeof(dest) - 1] = '\0'; printf("src = %s\ndest = %s\n", src, dest); } make test ./test src = Gobblygook is a long string dest = Gobb
- pjc50 11y ago
  While I've done this myself, it's not a 100% solution, because now you get a truncated string that further processing may not be expecting. For example, if you validate the un-truncated string somehow then take a copy of it, the copy may be invalid.
tobiasu 11y ago
This beard has its own beards.
JustSomeNobody 11y ago
What do people get out of pedantic rants such as this? Anyone who's written anything in C is quite aware of the issue here. And then there's this gem: "...If the source string is 5 characters long, and the target is a 1024-byte buffer, and you set n to the size of the target, strncpy will copy those 5 characters and then fill all 1019 remaining bytes in the target with null characters. Since all it takes to terminate a string is a single null character, this is almost always a waste of time. Ok, so that's not so bad. CPUs are fast these days, and filling a buffer with zeros is not an expensive operation, right? Unless you're doing it a few billion times, but let's not worry about premature optimization." Does he think languages with built in String types have some magical optimization juice that make string operations fast? How long does it take to instantiate a new String object, run it's copy constructor, blah blah. Any C programmer looks to avoid these types of situations.
- _kst_ 11y ago
  > What do people get out of pedantic rants such as this? Anyone who's written anything in C is quite aware of the issue here. Unfortunately, that's not the case. I presume this link was posted here as a result of my recent comment on the "Don't Learn C the Wrong Way" article, which recommends strncpy() and strlcpy() as safer alternatives to strcpy(). I've see a number of calls to strncpy(). I've rarely seen such calls explicitly null-terminate the destination array. I've even seen things like strncpy(dest, src, strlen(src)); which is certainly no safer than strcpy(). What I get out of this particular pedantic rant is an opportunity to let C programmers know that strncpy() isn't what they might thing it is (as well as a mention on the front page of Hacker News!).
- _kst_ 11y ago
  > Does he think languages with built in String types have some magical optimization juice that make string operations fast? How long does it take to instantiate a new String object, run it's copy constructor, blah blah. No, I don't think that. I didn't mention it because I wasn't talking about languages with built-in String types; I was talking about C.
bitwize 11y ago
You should be using strcpy_s...

56 points by luu 11y ago

lawl 11y ago

tl;dr: strncpy() doesn't null terminate, but doesn't mention strlcpy() http://www.openbsd.org/cgi-bin/man.cgi?query=strlcpy http://www.openbsd.org/cgi-bin/man.cgi?query=strlcpy

masklinn 11y ago
Because, much like reallocarray, strlcpy is not part of the C standard library. The correct solution, assuming you can't just import libbsd or include its functions, is probably to use snprintf. It goes guarantee null-termination of the target. So strncpy(dst, src, size_dst); becomes snprintf(dst, size_dst, "%s", src); Checking for partial copy is not ideal though, snprintf returns the number of bytes it would have copied had the destination buffer been infinite excluding the final NUL byte. So a copy is only complete if `ret < size_dst`. snprintf is not available in C89 though...
- rkangel 11y ago
  Or just to add an implementation of strlcpy in a util module in your codebase. e.g.: http://mirror.fsf.org/pmon2000/3.x/src/sdk/libc/string/strlcpy.c http://mirror.fsf.org/pmon2000/3.x/src/sdk/libc/string/strlc...
  - masklinn 11y ago
    Hence > assuming you can't just import libbsd or include its functions
    - lmm 11y ago
      Under what circumstances would you ever be unable to copy such a small and liberally-licensed file into your source?
      - nitrogen 11y ago
        If, for whatever reason, you don't want to have anyone else's copyright on your product, or don't want to keep track of which licenses have to be displayed where.
- jjnoakes 11y ago
  snprintf is also slower. It has to parse the format string. Just use strncpy and NUL-terminate the buffer. It's not hard. Write a two line function to do it for you, and use that function in place of strncpy.
  - _kst_ 11y ago
    dest[0] = '\0'; strncat(dest, src, size);
    - jjnoakes 11y ago
      Sure. Or: strncpy(dest, src, size); dest[size - 1] = '\0';
      - nitrogen 11y ago
        The fact that strncpy zeros the buffer may make it slower.
        jjnoakes 11y ago
        Sure, by a couple of cycles. Unless your size is huge. In which case, perhaps you should try to avoid copying strings in the first place.
  - masklinn 11y ago
    > Just use strncpy and NUL-terminate the buffer. It's not hard. Just as it's not hard to not dereference null pointers, not double-free allocations and a truckload of other mindless drudgery which history shows humans won't reliably get right all the time. Not to mention checking for copy truncation with strncpy is error-prone, you've got to check whether it points to a `\0` before you manually null-terminate it, providing more chances to get it wrong. > snprintf is also slower. It has to parse the format string. How about worrying about that when snprintf actually shows up in profiles and there's no algorithmic way to improve the situation and strncpy is a significant improvement?
MrBuddyCasino 11y ago
I'm not a C programmer, but even I knew that strlcpy() and snprintf() would come up in this discussion. How fucked up is that? Is there a resource where these issues are answered once and for all, for the novice C programmer? Also, are there deprecation warnings emitted by the compiler for non-safe C stdlib functions?
- clarry 11y ago
  On OpenBSD the linker whines about uses of unsafe functions. I don't see it becoming mainstream though, as there are so many people who disagree about what's unsafe.
- lawl 11y ago
  Not a C programmer either. But I've seen projects banning strcpy. The code was littered with strncpy(a, b, strlen(b)). So I'm not sure how useful these warnings would be.

cremno 11y ago

A similar article: https://randomascii.wordpress.com/2013/04/03/stop-using-strncpy-already/ https://randomascii.wordpress.com/2013/04/03/stop-using-strn... This article is also on GH (including some changes): https://github.com/Keith-S-Thompson/the-flat-trantor-society/blob/master/002-strncpy.md https://github.com/Keith-S-Thompson/the-flat-trantor-society...

StavrosK 11y ago

> That second paragraph means that if the string pointed to by s2 is shorter than n characters, it doesn't just copy n characters and add a terminating null character, which is what you'd expect. But what I would expect is that it would copy len(s2) characters, not n.

deleted 11y ago
[deleted]
patal 11y ago
That's correct. I think the author meant that.

JulianMorrison 11y ago

C people, isn't it perhaps time to quietly pasture the existing standard library and create a new one, from scratch, that isn't chock full of booby traps in the small print?

pjmlp 11y ago
The real problem, how C sees strings, arrays and pointers would still persist. Implicit conversion between vectors and pointers should have never happened. I don't get why people find so hard to write &vector[0] for those cases where a pointer is needed. Also if other languages, for other OS, older than C could do bound checking elision, C not doing it was only saving work for the compiler developer.
- kazinator 11y ago
  If &array[0] were needed, it wouldn't solve anything. The semantics that you can take the pointer to the first element, and pass it around and treat it as an array, with no meta-data connecting it to the original properties of the array, would still be there. The requirement for a cast would be better, because that's the existing notation for explicitly requesting conversions that are identified as unsafe. I.e. char array[42]; char *p = array; /* error, implicit conversion */ char *q = (char *) array; /* OK */ char *s = &array[0]; /* Sneaky: OK, without a cast */
  - pjmlp 11y ago
    Sure, but I was specially thinking about the issue of passing vectors into functions. In any way, for me the right way of doing systems programming is the way of Mesa, Modula-2 and many others. C was already bad in the 70's.
    - kazinator 11y ago
      I programmed quite a bit in Modula-2 before C. It didn't teach me enough to recognize the silliness in C. I believe that this is an aspect of C popularity that hasn't received a lot of recognition. Programmers were sucked away from cleaner languages into C, because the users of the cleaner languages had neglected to acquire the body of rationalizations for why those languages are that way which could have helped them stick. C looked good in naive ways. "Hey, look at all the things you can easily do with memory and pointers! Wow, look at that slick syntax: you can stuff several increments and an assignment into an expression, and use that as the test of a loop, ... man I'm never writing foo := foo + 4; again!" Modula 2's memory management is basically malloc/free though. While the language could save you from array overruns, it is basically defenseless against leaks and use-after-free bugs, as well as uses of unchecked null pointers under OOM: See: http://www.modula2.org/reference/isomodules/isomodule.php?file=STORAGE.DEF http://www.modula2.org/reference/isomodules/isomodule.php?fi... Look, the allocator works with generic pointers, has an explicit DEALLOCATE, and ALLOCATE returns NIL when there is no memory! Modula 2' standard also has a SYSTEM module which adds C-like things: pointer arithmetic, casting and whatnot. http://www.modula2.org/reference/isomodules/isomodule.php?file=SYSTEM.DEF http://www.modula2.org/reference/isomodules/isomodule.php?fi... Is that just "C envy", or is there perhaps need for those things, know what I mean?

angry_octet 11y ago

For someone being so pedantic it is puzzling that he doesn't address the actual title of his post. No one claims that strncpy is 'safe' they just say 'safer', that is, less likely than normal strcpy to have catastrophic bugs. And I think that claim stands up: make sure your n in strncpy is the length of your buffer, do a strncpy, write a null at the end of the buffer. Voila, all those strcpyies from unexpectedly large strings writing in your heap are gone. No need to constantly strlen source strings to check the length -- which in itself can cause a segfault if they are not null terminated. So yes, it is safer. You would be mad not to use it.

unwind 11y ago
Replacing one function call with a function call to a function that does almost the right thing in many cases, but potentially wastes time by pointlessly zero-filling a buffer, and an extra assigment statement is not an improvement. You would be mad to do that.
kazinator 11y ago
Starting in C99, you can do this: snprintf(buffer, size, "%s", source); This can be put into a #define macro or inline function. If what you want is a null terminated, truncated copy that fits into a given buffer size, and snprintf is available, you are mad to still use strncpy. Also, strncpy pads the destination with nulls, if there is remaining space. Because of this, the evident purpose of strncpy is to fill fixed-width text fields in "old school" fixed-field database records. Writing the extra nulls is wasteful if the data type in question is nothing but a C string.
- jedbrown 11y ago
  With fixed-width records, writing the extra nulls may be important for data privacy reasons and in any case, it allows comparison using memcpy(3), diff(1), and checksums.
  - _kst_ 11y ago
    Sure, but then you have to make sure that you always maintain the extra nulls. diff(1) doesn't seem relevant; it operates on text files, which normally don't contain any null characters.

useerup 11y ago

Microsoft has a number of dangerous c functions that are on the Secure Development Lifecycle "banned" list. And yes, strncpy is one of them. Some insights here: https://msdn.microsoft.com/en-us/library/bb288454.aspx https://msdn.microsoft.com/en-us/library/bb288454.aspx strncpy example vulnerability experience linked to from that article "Buffer Overflow in Apache 1.3.xx fixed on Bugtraq - the evils of strncpy and strncat!": http://blogs.msdn.com/b/michael_howard/archive/2004/10/29/249713.aspx http://blogs.msdn.com/b/michael_howard/archive/2004/10/29/24... Source code for MS products are automatically (since SDL) screened for use of the banned functions. You'll have to use the "safer" alternatives.

deleted 11y ago
[deleted]

earlz 11y ago

It's a shame that strncpy doesn't have the semantics of OpenBSD's strlcpy, which I think behaves as everyone would expect. http://www.openbsd.org/cgi-bin/man.cgi?query=strlcpy http://www.openbsd.org/cgi-bin/man.cgi?query=strlcpy

gvb 11y ago

There is a very simple, efficient solution: always put a null termination character in the end position of the destination after the strncpy(). If the source is longer than the destination, it will properly terminate the destination. If it isn't longer, it didn't cost (hardly) anything. Unfortunately, it isn't automatic so some times humans will forget to do it. #include <stdio.h> #include <string.h> int main(int argc, char *argv[]) { char *src = "Gobblygook is a long string"; char dest[5]; strncpy(dest, src, sizeof(dest)); dest[sizeof(dest) - 1] = '\0'; printf("src = %s\ndest = %s\n", src, dest); } make test ./test src = Gobblygook is a long string dest = Gobb

pjc50 11y ago
While I've done this myself, it's not a 100% solution, because now you get a truncated string that further processing may not be expecting. For example, if you validate the un-truncated string somehow then take a copy of it, the copy may be invalid.

tobiasu 11y ago

This beard has its own beards.

JustSomeNobody 11y ago

What do people get out of pedantic rants such as this? Anyone who's written anything in C is quite aware of the issue here. And then there's this gem: "...If the source string is 5 characters long, and the target is a 1024-byte buffer, and you set n to the size of the target, strncpy will copy those 5 characters and then fill all 1019 remaining bytes in the target with null characters. Since all it takes to terminate a string is a single null character, this is almost always a waste of time. Ok, so that's not so bad. CPUs are fast these days, and filling a buffer with zeros is not an expensive operation, right? Unless you're doing it a few billion times, but let's not worry about premature optimization." Does he think languages with built in String types have some magical optimization juice that make string operations fast? How long does it take to instantiate a new String object, run it's copy constructor, blah blah. Any C programmer looks to avoid these types of situations.

_kst_ 11y ago
> What do people get out of pedantic rants such as this? Anyone who's written anything in C is quite aware of the issue here. Unfortunately, that's not the case. I presume this link was posted here as a result of my recent comment on the "Don't Learn C the Wrong Way" article, which recommends strncpy() and strlcpy() as safer alternatives to strcpy(). I've see a number of calls to strncpy(). I've rarely seen such calls explicitly null-terminate the destination array. I've even seen things like strncpy(dest, src, strlen(src)); which is certainly no safer than strcpy(). What I get out of this particular pedantic rant is an opportunity to let C programmers know that strncpy() isn't what they might thing it is (as well as a mention on the front page of Hacker News!).
_kst_ 11y ago
> Does he think languages with built in String types have some magical optimization juice that make string operations fast? How long does it take to instantiate a new String object, run it's copy constructor, blah blah. No, I don't think that. I didn't mention it because I wasn't talking about languages with built-in String types; I was talking about C.

bitwize 11y ago

You should be using strcpy_s...