6 ms·
It is wrong in many ways. 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
by int0x80 8y ago
It is wrong in many ways.
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.
- jmts 8y agoNot sure why you're being downvoted. That's all pretty standard. 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.
- saagarjha 8y agoI'd suggest not insulting the people reading your comment if you want some points back…
- int0x80 8y agoYou are right. Fixed.
- simias 8y agoYou're right, although any mention of strncpy should come with a big disclaimer that it's effectively broken because you can end up with a non-NULL terminated string in some cases. strlcpy should be the way to go but unfortunately it's not part of the C standard and not available everywhere (sometimes for rather bullshit reason IMO, but that's a different story).
- jmts 8y agoDocumented behaviour is a little different to "effectively broken". The difference between strncpy and strlcpy is that strlcpy will NUL terminate the last byte for you always. There is nothing stopping you from doing the same thing yourself when you use strncpy. If you care enough, write your own strlcpy - it's only one extra line.
- simias 8y agoI'm not saying it's hard to work around but I maintain it's broken. You have a function that deals with C-string that in some conditions returns something that's not a C-string but can't be trivially distinguished from one and will trigger undefined behavior if used like one. It's terrible ergonomics and almost certainly not what you want to do in any situation. 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.
- caf 8y agoIt's not broken, but it is misnamed. 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/daa355109625a50e6b1080184dee30c9136549d1/ken/nami.c#L72 https://github.com/hephaex/unix-v6/blob/daa355109625a50e6b10...
- deleted 8y ago[deleted]
- kevin_thibedeau 8y agoFriends don't let friends use strncpy(). It doesn't guarantee NUL termination and wastes time zero-padding when it is rarely desired. Use C11 strncpy_s() or platform specific equivalents.
- int0x80 8y agoAgree.
- krylon 8y ago> snprintf (C99+) 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. ;-)