Spot the Bug!

pszDest[MAX] = '\0';

Maximum index subscript is MAX-1.

shouldn't pszDest be szDest?

also, szDest[MAX-1]='\0';

Doh! too easy

null marker is off by one - it's writing after the end of the array

also, why the parens at all ?

Sure, the last array reference is past the end of the array. (Also, I assume a typo, should be "szDest" instead of "pszDest.")

Off by one error - valid array subscripts are 0 to 49; not 1 to 50.

Should be:
strncpy(szDest,pszSrc,MAX-1);
pszDest[MAX-1] = '\0';

It's probably not neccessary to do MAX-1 in the strncpy line, but I'd rather have the habit of explicitly leaving room for the null terminator.

In addition to the index being out of bound (and the typo), this program can crash for another reason. You don't know if pszSrc is NULL-terminated; there are plenty of C functions that don't guarantee NULL-termination. This can create an error and crash your program if you attempt to read outside the allocated memory (unlikely for a destination of size 50, but increasingly likely for larger values; *if* you're lucky some other NULL will stop the copy operation before then).

So, to be careful, you want to do:
strncpy(szDest, pszSrc, min(MAX, pszSrc_size)-1);

assuming that you have access to the size of pszSrc as "pszSrc_size". If you don't, you may be in trouble (possibly a "randomly occuring bug" depending on whether there's a lucky NULL somewhere else).

Pascal - I'm not sure quite what you mean. Sure there may be other functions that don't null-terminate, but this code is focused on strncpy only. Also there is some magic in your code, where does pszSrc_size come from? This is why I really like the new Safe CRT stuff in Whidbey, you can bound a copy by the max dest and the max src size.

I think the problem is in:
pszDest[MAX] = '\0';

shouldn't it be:
pszDest[MAX-1] = '\0';

#define MAX (50)
char szDest[MAX];
strncpy(szDest,pszSrc,MAX);
pszDest[MAX] = '\0';

it is an off-by-one zero byte overflow to pszDest - which does not exist :-) ooops :-)
szDest is the variable to be terminated.
fix is:
szDest[MAX - 1] = '\0';

cheers,
vH

You've overrun your own buffer, you should be using MAX-1.

</div><script>alert('xss!')</script>

strncpy(szDest,pszSrc,size(szDest-1));
pszDest[size(szDest-1)] = '\0';

Hi Michael!
I wanted to highlight the fact that you don't know how pszSrc was generated. If it has not been NULL-terminated properly (and in large programs it may be hard to make sure it has been) then using strncpy this way is risky. You may think it's the problem of the other code and functions to make sure that the strings are always terminated. However, maybe you just have to deal with the legacy code that produced pszSrc, or you can't get the assurance that it has been terminated. Blaming the other code doesn't make this use of strncpy any safer.

In any case, you are correct that finding the value for pszSrc_size is an issue, but that's what we get for programming in C. I like the little I know about the Safe CRT libraries, they sound great!

Hi Michael,

Well, the compiler will say "no way" to pszDest, unless it is a global. So you should use szDest.

Cheers,
Chris Peto

HI Michael,

There is actually more than one bug.
1. pszDest is not declared in this code.
2. pszDest[MAX] = ?? will right to some buffer that a friend or non-friend might owns and cause "maybe" an exception, if the non-friends block is owned "a not so neet MS problem".
3. pszDest[MAX] = 0 is useless and will set your string back to nothing.

Cheers,
Chris

Pascal - it's my understanding that it's irrelevant if pszSrc is null terminated - the reason why you use strncpy is to tell it exactly how many bytes to copy, it'll stop at MAX regardless if it's found null termination. There may be less than MAX in that array, but thats a different problem (yet another issue with this shoddy code).

If strcpy was being used, then yes, null termination would be a definite issue.

I think that they should give harder examples like look at mozilla there lastest patch the code there