Remove security flaws in my code with help of splint

Question

Can anybody help me to fix this code? I don't understand much as I'm new to C. I'm using Splint to find security flaws in the code.

char *stringcopy(char *str1, char *str2)
{
    while (*str2)
        *str1++ = *str2++;

    return str2;
}

main(int argc, char **argv)
{

    char *buffer = (char *)malloc(16 * sizeof(char));

    stringcopy(buffer, argv[1]);

    printf("%s\n", buffer);
}

Splint output

@Mast I didn't tell the OP to migrate it to CR, that was someone else. It might be a better fit for SO indeed, if changed to a [mcve] and have the lint log posted as text. We shouldn't leave users in limbo between two sites. I'll retract my close vote. — Lundin, Oct 30 '18 at 15:04
@PerKristianGravdal I'm sorry that you are getting contradicting advise in comments, we seem a bit confused :) Please do the following to fix the post here on SO (click "edit"): 1) Edit the code here on SO by including the full version that you used to compile, including `#include` directives. If the compiler gave you any warnings/errors, please post those too. 2) Replace the Lint message picture with text. You should be able to copy/paste from the terminal. 3) Please mention which compiler that is used, as it seems to be an antique one. — Lundin, Oct 30 '18 at 15:10
I find it interesting that the long complaint from splint does not seem to mention the key safety issue here, which is that `stringcopy` does nothing to avoid a buffer overrun. Of course, that is an API design issue, but it is also the well-known problem with `strcpy` which I suppose this function is intended to replace. (The lack of NUL-termination is another problem. splint doesn't detect it either.) Personally, I'd suggest leaving this tool for later and concentrate on writing correct code. — rici, Oct 30 '18 at 16:29
@rici I wouldsay that it clearly isn't in `stringcopy()`'s mandate to detect or prevent that, but the calling code's. Not that it makes the situation any better. — Deduplicator, Oct 30 '18 at 17:06
@deduplicator: i agree, but I don't think that changes my issue with the output of splint. — rici, Oct 30 '18 at 17:12

Deduplicator · Answer 1 · 2018-10-30T17:07:43.037

You are missing the includes.
Your stringcopy() does not terminate the destination.
The source should really be a const char* to allow for const-correctness and let the compiler help catch bugs.
stringcopy() expects the destination to be big enough. Does 16 Bytes qualify for that in main()?
Consider packaging allocating and copying a string into one function, well-known as strdup().
For some reason, standard strcpy() returns a pointer to the destination. Yes, it's a good idea to return a pointer to the trminator instead, but consider following existing practice when naming the function to avoid unpleasant surprises.
Do not cast the result of malloc().
Additionally, use sizeof *pointer instead of sizeof(TYPE), it reduces unchecked repetition and avoids bugs.
Do not assume success. malloc() can always fail.
Generally, you should free() what you malloc(). As the program terminates immediately though, it would be useless make-work.
Implicit int is deprecated in C90, and removed in C99.
Implicit return 0; for main() appeared in C99, earlier it was Undefined Behavior (UB). But you already used implicit int, which was removed at that time. What shall it be?
Even though this isn't code-review, I earnestly recommend you put a bit more work into properly naming your parameters. Anyone reading your code for any reason (which is mostly you yourself at the moment) will be thankful. That does not mean the names should be longer though.

score 0 · Answer 2 · answered Oct 30 '18 at 15:18

Your stringcopy does not terminate the copied string. Besides, there is not much use in returning the end of the source string. The following is a suggestion:

char *stringcopy(char *str1, char *str2)
{
    char *s2= str2;
    while (*s2)
        *str1++ = *s2++;

    *s2= '\0';
    return str2;
}

Remove security flaws in my code with help of splint

2 Answers2