4 ms·
Most of these points are covered by the other comments. As a C programmer professionally, I'll go into a little more depth, and offer an alternative implementat
by jmts 8y ago
Most of these points are covered by the other comments. As a C programmer professionally, I'll go into a little more depth, and offer an alternative implementation for comparison.
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);
}
1. The array 'r' is allocated on the stack, and returned from the function. This is bad because 'r' goes out of scope as soon as the function is returned. This function returns a pointer to memory with essentially unknown contents.
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;
}
- caf 8y agoYou can avoid the need for setting the null terminator explicitly by using memcpy(str + slen, t, tlen + 1) as the second memcpy.
- jmts 8y agoAnd this is why we have code review ;)
- huhtenberg 8y agoif (NULL == s) Ouch. 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.
- maskros 8y agoBetter yet, just use the fact that NULL is false and get shorter, cleaner, and safer code. if (!s)
- TickleSteve 8y agoGod no... explicit is better than implicit. Your statement there just does not read correctly, always have the condition explicit.
- 08-15 8y agoWhy? It reads nicely as "if there is no string", can't get more explicit than that.
- jmts 8y agoThe biggest problem I have with this style is that it is inconsistent across variable types and provides no insight into the actual operation being performed without further knowledge. My personal experience is that my ability to read and comprehend code quickly is dependent on the context and patterns present within it (among other things). Therefore making the expression explicit as a pattern adds context and therefore increases readability and comprehension. 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.