Can someone please elaborate why it is bad? Are their any good resources to fill gaps in my knowledge?
Thanks in advance.
Edit: Thank you guys for pointing out so many problems. It seems that I have a lot to learn. :)
It copies s and then t to a fixed size buffer, without any checks. That will write to invalid memory (probably smashing the stack) if len(s), len(t) or len(s) + len(t) > 100.
It returns a stack allocated buffer (r) pointer to the caller. The array will be invalid when the function returns, as the automatic variables only live in the function scope (during the call), they are deallocated when the function returns.
To do this right you have various strategies.
1. Allocate a buffer of len(s) + len(t) + 1 with malloc, copy the strings and return it. Have the caller free it when it's done. It can be inapropiate because of the dynamic allocation.
2. Have the caller pass a destination buffer and its size. If you know the char *'s are zero terminated, check if you have space for them in the dest buffer. If not, truncate or error out. Most of the times, this is the prefered solution.
3. Use a static local buffer, and return it to the caller. You may need to truncate the copy too. Not recomended. This is not a good solution as the function also will not be reentrant (unsafe with multiple threads).
You can use libc functions like strncpy (C89+), snprintf (C99+) etc to make a "size-checked" copy with various automatic truncation semantics. You can refer to their man pages for details.
Edit: to the downvoters, please point out what is wrong in the comment.
Method 1 is a contract you often make as part of defining the interface. For an example, such things are out of scope so this is a perfectly reasonable point.
Method 2 is common. Most of the strn*() and snprintf() etc, do this. This is the preferred method for some of my colleagues.
Method 3 is used in the standard library, though as mentioned is usually avoided.
You can argue that truncation is an error condition but then it ought to notify it somehow, for instance by returning NULL in such a case. And even then it's incoherent with snprintf which doesn't have the same behaviour and does always terminate with '\0' even in case of truncation (assuming non-0 buffer length, of course).
It's just an unnecessary footgun that serves no practical purpose. It would be like a date function that gives you the today's date except on the 4th of December where it replies that it's the 31st of February. Not hard to work around but still broken.
This is because it is not intended to work with the same kind of string that the other str* functions work with (ie. an ordinary null terminated string).
Instead it's supposed to work with fixed-width string fields that pad out values shorter than the field width with nulls. This is how original UNIX directory entries were stored.
See how the name is copied into u.u_dbuf here: https://github.com/hephaex/unix-v6/blob/daa355109625a50e6b10...
The strncpy function writes to the entire buffer. This is important if you will be passing the buffer across a security boundary, for example in a network packet or as a struct copied into a publicly visible file. If trailing bytes are not cleared, then secret data (which happened to be sitting in memory) can get leaked.
I hate this mentality in a lot of C circles that boils down to "there's no bad language, just bad programmers, man up pussy". I like C, I use it a lot, it's one of the first languages I learned and it's been my main "professional" language for more than a decade. Yet I can also see that it has many unnecessary sore points. Having switch not break by default, gets(), array shenanigans, some aliasing rules, the hundreds of completely different meanings for "static", macro hygiene and I could go on... You can say "it's not a big deal and it's not going to change at that point anyway" and sure, I'm not arguing for a revolution, but let's not act that it's an absolutely perfect language and I'm an idiot that doesn't get it for pointing out these issues.
Principle of least surprise. Idiot proof. Semantics that match common usage. Call it what you like.
I think the API designer was trying to be parsimonious and not assume too much about what you want to do with the data in your destination buffer. The requirement was to provide buffer overrun protection when the source string is too large, and this function provides that. Beyond that, the requirement that the character array be null-terminated is your decision.
In fact, if you determine that it isn't null-terminated you can do other things besides null terminate it yourself. You might want to provide a helpful error message to consumers of your API rather than truncating their input data silently. Or you may try reallocating your buffer until the string fits. There's actual error handling that you can do with this function. In contrast, automatically null terminating the string makes that more difficult.
The other issues that you have with C seem like preferences. There's nothing wrong with not breaking by default in a switch statement as long as you know that that's what happens.
The linux manual for strncpy makes few good notes about its usage and that has a meaning, since it is official and will not change due someone ranting about something again.
Can you come up with an example of an API which you would consider to be effectively broken and yet is not actually broken? Presumably, it would be an API that's easier to misuse than strncpy.
I've written about strncpy: http://the-flat-trantor-society.blogspot.com/2012/03/no-strn...
It's been years since I have written C professionally (and I only did it for two years), but I felt that snprintf was the single biggest improvement that C99 brought to the table, at least for people that had to deal with strings. ;-)
1. Returns pointer to stack-allocated data, which immediately becomes invalid. Instead, it should be using some sort of allocation (e.g. 'malloc'), or taking in a destination pointer.
2. 'r' is arbitrarily set with length 100. Smaller strings don't need all that space, and larger strings definitely will overrun.
3. The function signature is really awkward. Without any of the surrounding textbook content, I'm not sure what behavior is supposed to happen. At first, I expected something like 'strcat', which takes two char* and appends the second one to the first one. But that isn't happening here and instead it seems to require dynamic allocation. (Hiding allocations inside a function is generally kind of weird. Usually the caller should be responsible for passing in a handle to the destination.)
4. There's no sensical limit on the loop iteration. If the input 't' doesn't have a null terminator, this is going to throw a ton of garbage into the stack space (because 'r' is stack-allocated to a fixed size). And also maybe run for a really long time.
5. 'strcpy' should usually be replaced by 'strncpy', which performs the same function but also requires you to provide a limit ("copy this string, but at most 'n' bytes"). That prevents a class of exploitable errors known as "buffer overruns". I don't know when the 'n' string functions were added to C or became popular, though.
This is a teaching exercise, so the fact that this is implemented as a separate function instead of calling 'strcat' from <string.h> doesn't seem like a big problem.
Then it's not a string.
To be honest, strncpy is barely better in this respect (as a security improvement) - truncating against arbitrary size limit in this day and age of text-only protocols... I wonder if outright crashing at the testing stage would be preferable rather than subtle misbehavior creeping into the release.
Both are bad IMO, the actual required buffer size should be known in advance.
Absolutely agree. Scanning the memory until "we find it", potentially crossing boundaries between segments of memory with different characteristics (caching etc.) just doesn't seem right in general, and if I recall, some CPUs even used to have published errata related to that.
> std::string` and the bafflingly just-introduced `std::string_view` are the right way to handle strings.
I'd even go straight to custom implementation of Hollerith strings. Literals have lengths known at compile time, protocols would either carry the lengths alongside the strings, or be trusted (to have good strlen behavior) until they do, composite strings would compute the length out of the components, etc. This doesn't seem too complex to do, looking from from my bell tower, but I know many people here would frown upon mentioning C++ in the context of embedded development (my area).
Sorry to butt in, but this is a bit of a trigger for me: I’ve had to fix a number of programs infected with this idea.
The main problems with strncpy are:
When the source string is shorter than n, strncpy will pad the target to n bytes, filling with zeros. This is bad for performance.
When the source string is longer than n, strncpy will copy n bytes but _not_ nul-terminate the target. So you need extra schenanigans every time you use it to cover this case.
So strncpy is hardly ever a good idea. Sadly there is no standard replacement that is widely accepted. More details at https://en.wikipedia.org/wiki/C_string_handling#Replacements
#define strlcpy(d, s, n) snprintf(d, n, "%s", s)
True, but practically: when having access to BSD extensions, strl* are used. If only C standard is available, snprintf is preferred. I have seen C libs that will check for strl* availability, and if not, reimplement them using snprintf.
So for portability, snprintf is the way to go. For correctness, and pushing for their extended use, strl* is nice.
Before writing to the buffer you should've ensured that it's big enough, and decided what to do if it's not, long before actually doing it. In other words, what happens if it's not big enough? These "always use $length_checking_function" proponents miss that point. Yes, you've avoided an overflow here, but chances are something was already too small long before the flow reached here, and the fix is not to replace an overflow with truncate/not copy/etc. here, but fix the check/sizing that came before elsewhere.
If you planned all this out, you're still making an assertion as to the length. The contract is "give me a string of this length" and if that's not enforced by the compiler, it ought to be enforced at runtime so that the error is detected and dealt with as soon as possible.
So maybe "safe string functions" should really be "fail fast string functions."
Even if it were, there's safety and there's safety. A function (like strncat, for example) that quietly truncates your data if it's too long isn't necessarily better than one that quietly ignores array overruns. Consider what happens if "rm -rf $HOME/tmpdir" is quietly truncated to "rm -rf $HOME/"
It returns a pointer to memory on the stack; but the stack will shrink when the function returns, and that piece of memory will get re-used. Which is consistent with the article's later claim that "I don’t think he understands the call stack."
Ok, so it's reasonable to think that this is supposed to be a teaching example and that considering these concepts might be a bit too early in the process. This leads into the second problem and one that is more subjective: this code is incredibly dense and relies on enough quirks of C that it's almost never going to be clear to a beginner reader what they are supposed to take away from it. It's maybe useful as a quiz question on C syntax and semantics, but there are enough barriers to understanding what the code is supposed to do that the amount of explaining the text would need to do to describe what the code is doing is most likely prohibitively long. Instructive examples should be unambiguous in what they are trying to show, otherwise students will be confused and potentially conflate issues in a way that is difficult to untangle later.
Edit: Ha! I spent so long looking at the first half of the function I totally missed that it was returning r! So, not only does this code have minor issues here and there from its careless implementation, it has a fundamental flaw that, if it were to work, would do so only by accident. I can only imagine that a student might walk away from this example thinking that C functions can return arrays and possibly misunderstand scoping in C.
> Not only does it not check, the design of the function means that there is no possible way for it to check safely. s and t are both pointers to characters? How long are the strings they might represent supposed to be?
strlen
> This leads into the second problem and one that is more subjective: this code is incredibly dense and relies on enough quirks of C that it's almost never going to be clear to a beginner reader what they are supposed to take away from it.
Most "nice" C functions are much terser than this.
Most "nice" C functions are also not used as teaching examples. There's a difference in how one writes C code for production use and instructive use.
void strcpy(char *s, char *t) {
while ((*s++ = *t++) != '\0') ;
} nullCharPtr - str
Sure you can say that good programmers will always ensure a C-string is a C-string but there's decades of programming history that shows that's not true in practice.
_String == 0 ? 0 : strnlen(_String, _MaxCount)* What happens if s or t are longer than 100 characters?
* What happens if s and t are both longer than 50 characters?
* What happens if no element of s == '\0'? How about t?
* r is allocated on the stack. What happens to the memory pointed at by r when you call another function after calling combine? Say you wanted to combine three strings; could you call combine twice to build up the result?
For the third point, you have two main ways of storing strings in general. The first was called pascal style, where the length is stored first in either a byte or two and then the string data with no (or an optional) terminating null character (the '\0'). The second is referred to as c style, and is done by storing the string in a memory and denoting the end of the buffer by a null character.
The c-style enables certain nicer looking c code with loops and such, but it is more dangerous and potentially expensive if you recompute the length all the time. You can always mix and match the way you store the string, such as in C++ where the string class stores a length as well as a null terminated string in a buffer.
For your forth point, yes, you end up corrupting the string leading to more crashes and/or it can be used as an entry point to screw around with your program's stack.
How so?
> I would have used strcpy 2 times
You can't do this, because strcpy doesn't give you the length of the string you copied, which is necessary to put the trailing null byte.
> The strcpy() function copies the string pointed to by src, including the terminating null byte ('\0'), to the buffer pointed to by dest.
But,yeah... It's a massacare.
How so?
This is wrong. Postfix increment has a higher precedence than the dereference operator.
Note that postfix increment is at the highest precedence and the dereference operator is at the second highest precedence.
The postfix operators behave in a bit of an interesting way; the expression "t++" evaluates to just "t" but increments t as a side effect. Consequently, it appears to update t after the expression it is part of, even though it is one of the first things evaluated.
If you actually test the function, you will find that it works provided that you make r static or global instead of a local stack variable -- and, of course, carefully mind the restriction on the length of the input strings to avoid overflowing the buffer.
In one sentence: this guy managed to reinvent a variant of strcat and get it completely wrong.
Here's a somewhat better version:
char *combine(char *s, char *t) {
size_t m = strlen(s), n = strlen(t);
char *ret = malloc(m + n + 1);
if(ret) {
memcpy(ret, s, m);
strcpy(ret + m, t);
}
return ret;
}If s and t are very long and overlapping, m + n could wrap around.
Furthermore, r is a fixed 100 bytes. There is no overflow checking whatsoever.
If you're actually learning C and writing programs in it then the two best things to do would be turn compiler warnings up to 11 and run valgrind often. So you want -Wall and -Wpedantic when you compile and if possible run with valgrind as part of your build script or run it with your tests, just run it often (the longer between runs the harder it is to track back to which change you made).
1 char *combine(s, t)
2 char *s, *t;
3 {
4
5 int x, y;
6 char r[100];
7
8 strcpy(r, s);
9 y = strlen(r);
10 for (x = y; *t != '\0'; ++x)
11 r[x] = *t++;
12
13 r[x] = '\0';
14
15 return(r);
16
17 }
1) The function is returning the address of a local variable. This alone makes this function rubbish.
2) The pointers s and t are unknown length, we have no guarantees that concatenating them will fit in a 100 character array. Also, he probably should be using malloc to dynamically allocate the space.
3) Use of the function strcpy rather than strncpy. He should have measured the length of s first, and then use strncpy if s was longer than 99 characters (don't forget the null terminator!). Then, rather than using a for loop, call strncpy again to copy the rest into a safe buffer size (the for loop is rather silly). The reason for this is that strcpy will cheerfully start copying past the array 'boundaries', and in this case, since he's copying into a local variable on the stack, is setting himself up for a Remote Code Execution attack if this ever gets untrusted input.
So those are the critical errors. These tie directly into why Geoff argues that the author doesn't understand the stack.
So let's, for educational purposes, go into this. We're going to go a bit into the weeds here. Sorry about that. This would be easier with a whiteboard. :)
When you fire up a program, the programs machine instructions get copied into memory, let's pretend at the memory location 0x1000. Far away from that code, at the highest memory values (more complicated on modern virtual systems, but hey, let's go back in time here :) ), the computer keeps track of a location called the stack pointer.
I'm going to put forward 3 diagrams now. Please forgive any off by one errors.
(Diagram a)
Registers
A 0
B 0
C 0
SP 0xffff
PC 0x1000
Address Mnemonic DATA
PC0x1000 MOV 1, A 0x00 0x01 0x01 0x01
0x1004 MOV A, C 0x00 0x01 0x03 0x01
0x1008 PUSH 3 0x01 0x00 0x00 0x03
... ...
... ...
... ...
0xfffc XXXXXXXX 0x00 0x00 0x00 0x00 <-- Stack Pointer is here
(Diagram b)
Registers
A 1
B 0
C 1
SP 0xfffe
PC 0x1000
Address Mnemonic DATA
0x1000 MOV 1, A 0x00 0x01 0x01 0x01
0x1004 MOV A, C 0x00 0x01 0x03 0x01
PC0x1008 PUSH 3 0x01 0x00 0x00 0x03
... ...
... ...
... ... v------\
0xfffc XXXXXXXX 0x00 0x00 0x00 0x03 ^-- Stack Pointer is here
void function() {
int a = 5;
int b = 2;
return a;
}
void main() {
return function();
}
(Diagram c)
Registers
A 5
B 0
C 0
SP 0xfff8
PC 0x2010
Address Mnemonic DATA
# Main starts here
# (Note, in C, there is actually code that gets executed before this)
PC0x1000 PUSH 0x1008 # We want to remember where to return to, so we push it to the stack.
0x1004 JMP 0x2000
0x1008 EXIT A # In this implementation of C, the A register will propagate return values
... ...
# Function 'function' is here
0x2000 PUSH 5 # Local variables go on the stack.
0x2004 PUSH 2
0x2008 MOV [SP+2], A # Locally, we refer to local variables by
# offsets to the stack pointer, so if this function
# were to call itself, the stack would keep growing down
# but these values would be good.
0x200c MOV SP+2, SP # Reset the stack before returning
PC0x2010 JMP #SP # Made up notation. Look at the value of the stack pointer, pop it, and jump to it.
# in x86, this is kinda what RET does.
... ...
... ... ...
... ... ...
0xfff8 XXXXXXXX 0x00 0x00 0x00 0x00SP
0xfffc XXXXXXXX 0x02 0x05 0x10 0x08
Furthermore, if the input strings are longer than expected, than they can overwrite values on the stack itself, including the return addres, causing your program to jump to some (if you're lucky) random location in memory.
Honestly, some of the best ways to get intuition for how the stack works, and the things that can go wrong, are CTFS at overthewire.org.
Also, https://microcorruption.com/
http://overthewire.org/wargames/bandit/
http://overthewire.org/wargames/leviathan/
[1] Sorry, 'down' means lower memory addresses, even though the displays of memory layouts always have lower memory addresses "up". :(
The function in question:
char *combine(s, t)
char *s, *t;
{
int x, y;
char r[100];
strcpy(r, s);
y = strlen(r);
for (x = y; *t != '\0'; ++x)
r[x] = *t++;
r[x] = '\0';
return(r);
}
2. The array 'r' is allocated at a fixed size. This is okay if you know ahead of time that you know this size, however for this function we don't know the lengths of s and t, so the odds that we are allocating the right amount of memory is slim.
3. As a result of points 1 and 2, the strcpy and loop may cause a buffer overflow. This is a class of bug where it is possible to overwrite memory that should be unavailable to us. In this case, if the combined lengths of s and t happen to be equal or greater than 100 characters, we will be overwriting memory that does not belong to r, corrupting it, and potentially crashing the program. This may additionally be as security risk, as buffer overflows can be exploited to execute malicious code.
4. There are no checks to see whether s and t are valid pointers. If they are NULL then the function would generate a segmentation fault. This is a check that is often ignored in cases where it is deemed to potentially hinder performance if the function is used frequently.
saulrh also mentions the case where s or t are not NUL-terminated. This is often considered to be a pre-condition of the function in C, suggesting that providing the function strings that aren't NUL terminated is an issue for the user.
For some comparison the following would be my first cut at the same function. Note I the comments are only for illustrative purposes. I'd omit them in actual code.
char* combine(const char *s, const char *t)
{
size_t slen, tlen;
char *str;
// optional checks for validity (point 4)
if (NULL == s || NULL == t)
{
return NULL;
}
// get lengths of s and t, to calculate allocation size
// also save them to use with memcpy later
slen = strlen(s);
tlen = strlen(t);
// allocate to heap (point 1)
// allocate correct size (point 2)
str = malloc(slen + tlen + 1);
if (NULL == str)
{
return NULL;
}
// use memcpy since we already know the size
memcpy(str, s, slen);
memcpy(str + slen, t, tlen);
str[slen + tlen] = '\0';
return str;
} if (NULL == s)
I haven't seen a single case where this abomination actually helped catching the fearsome 'if (s = NULL)' typo. One needs to be a sloppy typist, not paying attention to what they write, not proof-reading the code before committing and ignoring compiler warnings for this disaster of a notation to be even remotely justified.
if (!s)Your statement there just does not read correctly, always have the condition explicit.
The statement '!x' tells me nothing about x, just that I'm expecting a given logical value. The semantic meaning of that value however, I have no idea. Odds are it is probably a NULL or 0, but that doesn't help much because (at least in the linux world) they can have different meanings despite having the same logical value. I cannot differentiate between success (!(x == NULL)) or failure (!(x == 0)) without more context.
The statement 'x == NULL' immediately tells me at a glance that I am dealing with a pointer, and therefore I should pay more attention to how it is used. I know I should now be looking for other patterns of safe/unsafe pointer management that are immediately relevant to the function I am reading right now. There is no need for me to look elsewhere and go on some tangent to find out, then to have to recall what I was doing when I come back later. I can immediately start considering the likelihood of segmentation faults, or other memory management issues. x == NULL also tells me that here I am expecting failure of some kind. The context around that condition should then tell me which kind, eg NULL == alloc_thing() vs NULL == find_thing().
Similarly, the statement 'x == -1' tells me I am working with an integer. This can tell me immediately whether I am expecting success (x == 0), failure (x == -1), or that I should make a mental note of more complex possibilities (THING_WORKED == do_thing()).
Code is a narrative. It is documenting the answers to the questions you are asking while writing it, and it should answer the questions that someone else is asking while reading it. When somebody asks you to explain something, the least helpful thing you can do is answer a plain "yes" or "no". It is better to give context and answer further related questions before they need to be asked. Readability and comprehension of code are no different - any given programmer is asking questions of the code. The more questions they have to ask, the longer it will take them to discover the answers in order to understand the code. Context helps. Shorter is not always better.
If you have to write “== true” everytime that’s your own idiosyncratic hangup. It has nothing to do with explicitness, it’s just redundant. The true test was explicit the moment you wrote if (
I actually recently saw an 'if (x = 0)' or similar get caught in review recently. Time pressure increases, tests are rushed, and authors proof-read the code in their head, and not what's on the screen. These things do happen, and if your personal style preferences get in the way of using a simple trick to save your own time at best case, or multiple other people's time at worst, then you might wish to reconsider your preferences - even if it is at 1 in 1000 odds.
I've seen weirder stuff get through the reviews and compile cleanly, something like "f,()". Typos happen, but (a) it's not a good enough reason to make the code less readable (b) if the code is prone to this sort of errors, just pay closer attention to them during the review phase. Hedging against a single exotic type of mistake that virtually never happens at the expense of code readability is unacceptable.