Coverity reports on possible overruns of static arrays

[Paul Vriens]

Hi,

we have quite a few places in the code where we do:

WCHAR param[any-value];

len = sizeof(param) / sizeof(WCHAR);

param[len] = '\0';

and of course more-or-less the same for CHAR arrays.

This could lead (and the example does) to writing behind the end of
param.

I've submitted two patches for this, but I'm not sure just doing:

param[len - 1] = '\0';

is the correct/good approach.

Any idea's?

Cheers,

Paul.

[Eric Pouech]

Paul Vriens a écrit :
> Hi,
>
> we have quite a few places in the code where we do:
>
> WCHAR param[any-value];
>
> len = sizeof(param) / sizeof(WCHAR);
>
> param[len] = '\0';
>
> and of course more-or-less the same for CHAR arrays.
>
> This could lead (and the example does) to writing behind the end of
> param.
>
> I've submitted two patches for this, but I'm not sure just doing:
>
> param[len - 1] = '\0';
>
> is the correct/good approach.

well, it depends what the rest of the code does...
anyway, param[len] is wrong...
use param[len-1] is the right situation but insure the other operations 
on the buffer (especially the ones before setting the terminating 
character) are done on a buffer of size len - 1

but you have to determine (on a case by case situation) if either the 
index of last index is really len - 1 in the rest of the code, OR if the 
programer didn't want a buffer of size any-value + 1, and forgot about 
the + 1 (but the rest of the code shall be looked at anyway)
A+

[Michael Stefaniuc]

Hey,

Paul Vriens wrote:
> we have quite a few places in the code where we do:
>
> WCHAR param[any-value];
>
> len = sizeof(param) / sizeof(WCHAR);

And there lies the next potential bug. If somebody changes the type of
param this will result in a wrong length. For this reason the Linux
Kernel guys are replacing all those constructs with a macro ARRAY_SIZE

#define ARRAY_SIZE(x) (sizeof(x)/sizeof((x)[0]))

Thought about doing that for Wine too but have to ask Alexandre first if
he would accept such patches.
> param[len] = '\0';
>
> and of course more-or-less the same for CHAR arrays.
>
> This could lead (and the example does) to writing behind the end of
> param.
>
> I've submitted two patches for this, but I'm not sure just doing:
>
> param[len - 1] = '\0';
>
> is the correct/good approach.
>
> Any idea's?

If this is such a common operation why not create a macro for that too.
Something like

#define ARRAY_ZERO_LAST(x) ((x)[ARRAY_SIZE(x)-1] = '\0')

bye
    michael