3 ms·

> My advice is to just write your own string data structures and functions. I would rather not have to do this, and I would prefer code that I'm reviewing not

> My advice is to just write your own string data structures and functions. I would rather not have to do this, and I would prefer code that I'm reviewing not do it either. C strings are error-prone, as you point out, and when I'm evaluating code for security issues, it's useful to know exactly how a function will behave in edge cases--is the buffer guaranteed to be terminated? is the return value the number of bytes written, like sprintf, or the number of bytes that could have been written, like snprintf? A new function with new behavior is twice the work: you have to verify it is consistent with the rules it claims to follow, and then make sure it is being used correctly. One memorable example was a convoluted routine that I suspected was the same as strdup() but I had to spend a while validating it. It's just a shame that so many of the standard library string functions fall short. This is the reason why there are so many security vulnerabilities, because there are too many nuances and inconsistencies. They messed up strcpy() so they introduced strncpy(), but they messed that up too so we had to create strlcpy(), which is not a drop-in replacement (and not portable apparently). So we've got 3 poor design choices implementing one trivial function, no clear replacement to reach for, and we expect every programmer to know this.

jstimpfle 8y ago
> is the return value the number of bytes written, like sprintf, or the number of bytes that could have been written, like snprintf? Both report the same number. snprintf just doesn't write more than n bytes. > when I'm evaluating code for security issues, it's useful to know exactly how a function will behave in edge cases simple data: no edge cases. Pointer + length, that's simple. The most complexity you might be facing is if you allow a NULL pointer in case of 0 length (which is common). There should be hardly any function that cares for that. Aside, C strings are still a valid alternate representation in some cases. For example, for many small strings (where an explicit length field would double the cost) or purely as a convenience in conjunction with string literals. Or for some serialized representations (it's nice not having to deal with compatibility problems concerning the physical representation of the length value). > This is the reason why there are so many security vulnerabilities, because there are too many nuances and inconsistencies. I think the reason is that the standard library is overused. Functions like strcat etc. are not only inefficient. They have a needlessly complex API. Would people create the functionality they actually needed instead of working around the ill-fitting API in each line of code, then there would be fewer vulnerabilities. > strcpy() [..] strncpy() [..] strlcpy [..] no clear replacement to reach for, and we expect every programmer to know this. No. Just don't use this stuff. It's too complex. It's the wrong interface. If you deal with strings and don't do automatic reallocation you need to get their lengths anyway. So what you should do is something like void do_silly_stuff(const char *a, const char *b, const char *c, int alen, int blen, int clen) { char buf[FIXEDSIZE]; ASSERT(alen + blen + clen <= sizeof buf); int len = 0; memcpy(buf + len, a, alen); len += alen; memcpy(buf + len, b, blen); len += blen; memcpy(buf + len, c, clen); len += clen; do_more_silly_stuff(buf, len); } Nice and explicit. You cannot get less dangerous in C. Or if you do automatic realloction ("dynamic string"): String sillyconcat(String a, String b, String c) { String result = new_string(); //optionally: // string_reserve(string_length(a) + string_length(b) + string_length(c)); string_append(result, a); string_append(result, b); string_append(result, c); return result; } ... but I'd only recommend this approach if you want to get super-comfortable and are willing to pay the cost of being maybe a bit opaque and of buying in into a specific String type.
- rgovostes 8y ago
  Ah, but even this can be dangerous. Suppose FIXEDSIZE = 4096. If you pass 715,827,883 as all three length parameters, your assertion (assuming it's not compiled out) passes, but a buffer overflow occurs. Basically, using the C string routines with untrusted input is terrifying.
  - jstimpfle 8y ago
    Yep. You could argue to death almost any line of C code that contains an arithmetic operation in this way. But the ASSERT is not supposed to catch everything. It's a basic protection against programming errors. (Realistically I rarely have strings that are 700 MB large). Unfortunately, arithmetic overflow doesn't result in an exception. I rarely want wraparound.
    - leetcrew 8y ago
      > Realistically I rarely have strings that are 700 MB large maybe not that long, but it's not that hard to lose a '\0' when serializing complicated data structures to disk. when you read the file back in, suddenly one of your structs contains an arbitrarily long string. i've seen several 32+MB strings get created this way.
spc476 8y ago
`strcpy()` and `strncpy()` were defined at the same time, but to understand `strncpy()` you have to understand the original Unix file system. Directories are special files that map names to inodes [1]. Back then, each entry in a directory was 16 bytes---14 for the name, and 2 bytes for the inode number. `strncpy()` was designed to slap filenames into the directory; adding a NUL byte would either restrict filenames to 13 bytes, or overwrite the inode number. Also remember, this was done in the early 70s, when space was at a premium and computers weren't networked with every other computer in the world. It was a different time then. The ANSI committee, when standardizing C, were trying to keep existing code valid when possible. Up until that time, you had a lot of different C libraries. Some defined `bzero()` to clear memory, others defined `memset()`. The C committee had to distill all these conflicting libraries so as to minimize the work each C vendor had to do to get a conforming C library. [1] Inodes contain all information about a file except its name. Still true today.
caf 8y ago
They messed up strcpy() so they introduced strncpy()... That's not actually the case. strncpy() wasn't introduced as some kind of "safer strcpy()", it was built specifically to deal with strings stored in the manner of original UNIX directory entries - a fixed-width array where any unused space is padded out with NULs. That's why it worked the way it did - such a string that filled the whole array with no NUL at all was still valid. The major problem with strncpy() was always its name - the "str" prefix was (obvious in hindsight) taken by people to mean that it dealt with ordinary C NUL-terminated strings, whereas the reality was that only its source argument was supposed to be such a string. It should have had a different name (this is illustrated perfectly by the similarly-named strncat(), which does deal only with ordinary C strings), but by the time of standardisation that ship had sailed. Probably it was niche enough that it just should have been left out of C89 altogether.

jstimpfle 8y ago

> is the return value the number of bytes written, like sprintf, or the number of bytes that could have been written, like snprintf? Both report the same number. snprintf just doesn't write more than n bytes. > when I'm evaluating code for security issues, it's useful to know exactly how a function will behave in edge cases simple data: no edge cases. Pointer + length, that's simple. The most complexity you might be facing is if you allow a NULL pointer in case of 0 length (which is common). There should be hardly any function that cares for that. Aside, C strings are still a valid alternate representation in some cases. For example, for many small strings (where an explicit length field would double the cost) or purely as a convenience in conjunction with string literals. Or for some serialized representations (it's nice not having to deal with compatibility problems concerning the physical representation of the length value). > This is the reason why there are so many security vulnerabilities, because there are too many nuances and inconsistencies. I think the reason is that the standard library is overused. Functions like strcat etc. are not only inefficient. They have a needlessly complex API. Would people create the functionality they actually needed instead of working around the ill-fitting API in each line of code, then there would be fewer vulnerabilities. > strcpy() [..] strncpy() [..] strlcpy [..] no clear replacement to reach for, and we expect every programmer to know this. No. Just don't use this stuff. It's too complex. It's the wrong interface. If you deal with strings and don't do automatic reallocation you need to get their lengths anyway. So what you should do is something like void do_silly_stuff(const char *a, const char *b, const char *c, int alen, int blen, int clen) { char buf[FIXEDSIZE]; ASSERT(alen + blen + clen <= sizeof buf); int len = 0; memcpy(buf + len, a, alen); len += alen; memcpy(buf + len, b, blen); len += blen; memcpy(buf + len, c, clen); len += clen; do_more_silly_stuff(buf, len); } Nice and explicit. You cannot get less dangerous in C. Or if you do automatic realloction ("dynamic string"): String sillyconcat(String a, String b, String c) { String result = new_string(); //optionally: // string_reserve(string_length(a) + string_length(b) + string_length(c)); string_append(result, a); string_append(result, b); string_append(result, c); return result; } ... but I'd only recommend this approach if you want to get super-comfortable and are willing to pay the cost of being maybe a bit opaque and of buying in into a specific String type.

rgovostes 8y ago
Ah, but even this can be dangerous. Suppose FIXEDSIZE = 4096. If you pass 715,827,883 as all three length parameters, your assertion (assuming it's not compiled out) passes, but a buffer overflow occurs. Basically, using the C string routines with untrusted input is terrifying.
- jstimpfle 8y ago
  Yep. You could argue to death almost any line of C code that contains an arithmetic operation in this way. But the ASSERT is not supposed to catch everything. It's a basic protection against programming errors. (Realistically I rarely have strings that are 700 MB large). Unfortunately, arithmetic overflow doesn't result in an exception. I rarely want wraparound.
  - leetcrew 8y ago
    > Realistically I rarely have strings that are 700 MB large maybe not that long, but it's not that hard to lose a '\0' when serializing complicated data structures to disk. when you read the file back in, suddenly one of your structs contains an arbitrarily long string. i've seen several 32+MB strings get created this way.

spc476 8y ago

`strcpy()` and `strncpy()` were defined at the same time, but to understand `strncpy()` you have to understand the original Unix file system. Directories are special files that map names to inodes [1]. Back then, each entry in a directory was 16 bytes---14 for the name, and 2 bytes for the inode number. `strncpy()` was designed to slap filenames into the directory; adding a NUL byte would either restrict filenames to 13 bytes, or overwrite the inode number. Also remember, this was done in the early 70s, when space was at a premium and computers weren't networked with every other computer in the world. It was a different time then. The ANSI committee, when standardizing C, were trying to keep existing code valid when possible. Up until that time, you had a lot of different C libraries. Some defined `bzero()` to clear memory, others defined `memset()`. The C committee had to distill all these conflicting libraries so as to minimize the work each C vendor had to do to get a conforming C library. [1] Inodes contain all information about a file except its name. Still true today.

caf 8y ago

They messed up strcpy() so they introduced strncpy()... That's not actually the case. strncpy() wasn't introduced as some kind of "safer strcpy()", it was built specifically to deal with strings stored in the manner of original UNIX directory entries - a fixed-width array where any unused space is padded out with NULs. That's why it worked the way it did - such a string that filled the whole array with no NUL at all was still valid. The major problem with strncpy() was always its name - the "str" prefix was (obvious in hindsight) taken by people to mean that it dealt with ordinary C NUL-terminated strings, whereas the reality was that only its source argument was supposed to be such a string. It should have had a different name (this is illustrated perfectly by the similarly-named strncat(), which does deal only with ordinary C strings), but by the time of standardisation that ship had sailed. Probably it was niche enough that it just should have been left out of C89 altogether.