views:

169

answers:

3

Hello all, I tried to search the site for this question but didn't find this exactly, although this subject is being discussed a lot...

I have this declaration in a cpp file, not within any function:

static const char* gText = "xxxxxxxxxxx";

Although it has a fixed size, I get a warning from a static analysis tool (Klocwork) when I'm trying to copy it to another char* variable - about possible out of bounds violation:

char xText[32];
SecureZeroMemory(xText, 32);
memcpy(xText, gText, strlen(gText));

Is it a false positive or is the global variable being initialized later?

Thanks!

A: 

It looks to be a false positive.

ckv
+1  A: 

It is a false positive. strlen is probably abstracted as returning an unknown positive number, so that when analyzing the pattern memcpy(dest,src,strlen(src)); the analyzer does not realize that the reading part of the copy is safe as soon as src is a well-formed string.

If you were using strcpy, the analyzer would probably conclude that it's okay in this case. Do you have a reason not to? The function strcpy is considered "unsafe" but your memcpy(..,src,strlen(src)) is quite unsafe too.

EDIT: Also, sellibitze raises a very good point in the comments: the const attribute in the original code only applies to the chars pointed by gText and not to gText itself.

Pascal Cuoq
thank you. strcpy doesn't solve this, but I conditioned the copying to be done only if the length is not bigger than the bounds. Thanks!
IUnknownPointer
+1  A: 

I would argue it is not a false positive. There is a potential risk that somebody could come along and change the length of gText without realising that it cannot be over 32 characters. I would definitely put some sort of check in before the memcpy to make sure there cannot be a buffer overrun.

e.g.

char xText[32];
SecureZeroMemory(xText, 32);
size_t lenToCopy = MIN(strlen(gText), 32);
memcpy(xText, gText, lenToCopy);

Also I'd replace the magic number 32 with a constant.

JeremyP
I commented on a very similar remark and the answer was deleted by its author. So I will not repeat the same thing again, but since you have decided to work with all possible values of `gText`, why assume that `gText` is a well-formed string (it is illegal to call `strlen` on a char array that is not a well-formed string)?
Pascal Cuoq
And the `32` in `MIN(..,32)` should be `31` if `xText` is supposed to contain a well-formed string after the copy.
Pascal Cuoq
@Pascal: The use of memcpy rather than a string function implies to me that the intention was to get something that would be treated as a fixed size block of bytes rather than a C string. That's why I set the limit to 32. Were I trying to duplicate a string, I would use strncpy (or equivalent) to do it - or as this is C++ std::string.
JeremyP
Thank you. But, as a BKM rule, don't do partial copies. Never. It may cause strange results which will be hard to find the root cause. return an error code\ throw an exception instead.
IUnknownPointer
@IUnknownPointer: JeremyP rule: programming can't be done successfully by sticking to rigid rules all the time. You need to use intelligence. They're not rules, they're more like guidelines.In this case, there was no clue as to your intent. Maybe you had some reason to want only the first 32 characters of a string to put in some header for some network protocol.
JeremyP
Thanks :) http://www.auditmypc.com/acronym/bkm.asp
IUnknownPointer