Tainted string message from Coverity using getenv

Question

Running Coverity on my code results in tainted string error message. I am using the "path" variable declared in the stack, so I am not sure why I am seeing errors. I can only think that using getenv() directly in the strncpy() is causing the error. Would the fix below eliminate this error?

char path[1024] = {NULL, };
if(getenv("A"))
    strncpy(path, getenv("A"), strlen(getenv("A")));

to

char path[1024] = {NULL, };
char * adriver = getenv("A");
if(adriver)
    strncpy(path, adriver, strlen(adriver));

Answer 1

No, this will probably not fix the error.

Coverity is telling you that the data within environment variable "A" could be pretty much anything; this data is not under the control of your program.

Therefore, you need to have some sanity checks on the data before you use it.

Your proposed fix will currently have a buffer overflow, if somebody sets environment variable A to a string containing 1025 characters.

Additionally, neither version of code will ever NUL-terminate the "path" string. This is because, you are using strncpy, which will not NUL-terminate if the byte limit is applied (which it will in this case, because you say "limit the copied string to the length I just got from the string").

What you should be doing, is size-check the string first. If it is too large, return some sort of error code; the path in variable A is too big, so your code will not function as desired. If it is not too large, copy it into the path buffer. If you want to use strncpy, make sure to leave room for a NUL at the end, and then explicitly add it, since strncpy is not guaranteed to put a NUL there.

Answer 2

char *path = getenv("A");

isn't that all you need? Is there a reason you need to make a copy of what getenv returns? If so,

char *path = NULL, *A = getenv("A");

if(A != NULL)
   {
   path = malloc(strlen(A)+1);
   strcpy(path, A);
   }

Answer 3

Would the fix below eliminate this error?

No, but this almost certainly would:

char *e = getenv("A");
char path[e ? strlen(e) + 1 : 1];
strcpy(path, e ? e : "");

The main problem with your code occurs when getenv("A") returns a string that's 1024 bytes or longer; copying this into path as you are results in a buffer overflow, as it isn't guaranteed that strlen(getenv("A")) will be less than or equal to 1024. You could fix this by using:

strncpy(path, getenv("A"), sizeof path); // NOT RECOMMENDED

... but then path won't be terminated with a '\0' character; path won't be a string in this situation, because by definition a string must be terminated with a '\0' character... This means you wouldn't be able to use it as a string.

That is what Coverity is most likely complaining about here, and my code eliminates the problem by using a VLA (variable length array) that's large enough to store the entire string, plus the '\0' at the end of the string.

Answer 4

Your code is incorrect: you have a potential buffer overflow in both alternatives.

I am not sure Coverity diagnoses the problem correctly, you did not post the exact error message. Coverity is possibly indicating that you use a string from the environment, that could have any length, potentially causing a buffer overflow when copied by your code into a 1024 byte buffer, indeed it is a good thing it pointed you to this. Here is why:

strncpy does not do what you think it does. This function should never be used, its semantics are error prone, it is not the right tool for your purpose. strncpy(dest, src, n) copies no more than n chars from src to dest and fills the rest of the array at dest with '\0' bytes until n bytes have been written. dest must point to an array of at least n chars. If src is shorter, the behavior is inefficient as the padding is usually unnecessary, but if src has a length of at least n, dest will not be null terminated by strncpy, leading to undefined behavior in many cases.

Your code:

char path[1024] = { NULL, };
if (getenv("A"))
    strncpy(path, getenv("A"), strlen(getenv("A")));

is equivalent to

char path[1024] = { NULL, };
if (getenv("A"))
    memcpy(path, getenv("A"), strlen(getenv("A")));

As you see, no real protection is granted.

You call getenv 3 times, it would indeed be more efficient to use your alternative implementation, but there are other problems:

You initialize path with { NULL, }. This is inconsistent and in many cases incorrect. NULL is commonly #defined as ((void*)0), thus an invalid initializer for char. path could be initialized this way:

char path[1024] = { 0 };

To avoid overflowing the destination buffer, use this code:

char path[1024] = { 0 };
char *p = getenv("A");
if (p != NULL) {
    strncat(path, p, sizeof(path) - 1);
}

But this would truncate the environment value, which may not be appropriate depending on how you use path.

An alternative is to use the environment value directly:

char *path = getenv("A");
if (path == NULL)
    path = "";

And if you change the environment values with setenv during the execution of your program, you might want to make a copy of the environment value with path = strdup(path);. This might also fix the tainted string warning from Coverity, although the copy would be the same size as the original and enough memory might not be available, which should be tested. Judging from Tainted string in C , it seems Coverity is a bit extreme about tainted strings. While the warning you get indicates a real problem, getting rid of the warning may sometimes require strange workarounds.