buffer too small for currency

[Fabian Cenedese]

Hi

I get this warning when I try to start a basic program. This comes from the
function VARIANT_GetLocalisedNumberChars. I added some printfs and
found that my currency is apparently "SFr.", so 4 chars plus zero which
is too much for the 4 char buffer. Strange though that on the command line
it looks differently:

locale -k LC_MONETARY
int_curr_symbol="CHF "
currency_symbol="Fr."
mon_decimal_point="."
mon_thousands_sep=" "
mon_grouping=3;3
...

Anyway, I thought I increase this buffer. But then the string gets stored
in two separate variables (each one char) in the VARIANT_NUMBER_CHARS
struct. Is there a reason that this is not a string? Why only two chars?

Thanks

bye  Fabi

[Fabian Cenedese]

> I get this warning when I try to start a basic program. This comes from the
> function VARIANT_GetLocalisedNumberChars. I added some printfs and
> found that my currency is apparently "SFr.", so 4 chars plus zero which
> is too much for the 4 char buffer.

I continued this one too (I don't like unhandled exceptions :)
I found the problem but not how to solve it. It's in the file locale.c
As it stands now the value gets copied even if the buffer is too small which
nicely destroys the stack.

static INT get_registry_locale_info( LPCWSTR value, LPWSTR buffer, INT len )
{
--snip--

if (!status)
    {
        ret = (size - info_size) / sizeof(WCHAR);
        /* append terminating null if needed */
        if (!ret || ((WCHAR *)info->Data)[ret-1])
        {
            if (ret < len || !buffer) ret++;
            else
            {
                SetLastError( ERROR_INSUFFICIENT_BUFFER );
                ret = 0;
            }
        }
        if (ret && buffer)
        {
            memcpy( buffer, info->Data, (ret-1) * sizeof(WCHAR) );
            buffer[ret-1] = 0;
        }

The found value (info->Data) should only be copied to (buffer) if its length (len)
is big enough. len is given in bytes (8 for the above call with a buffer of 4 WCHARs).
But the length of the data (ret) is in WCHAR units. So the comparison here
is completely wrong. Ok, I could change this to len/sizeof(WCHAR). But that's
still not enough because of the comparison before about info->Data[ret-1].

ret is calculated as 5 for the string "SFr.". I don't know if it's correct that it
includes the ending null. If it is then the test needs to be on info->Data[ret-2].
But if the size should be 4 somebody else needs to check which of these
(partly undocumented) functions should return a different size.

If I don't hear anything I'll take the easy road and send in a patch :)

Thanks

bye  Fabi

[Uwe Bonnes]

> > > > > "Fabian" == Fabian Cenedese  writes:

Fabian> If I don't hear anything I'll take the easy road and send in a
    Fabian> patch :)

As always, it is a good idea to write a testcase in dlls/kernel/tests to
document this error...

Bye

[Fabian Cenedese]

> Fabian> If I don't hear anything I'll take the easy road and send in a
>    Fabian> patch :)
>
> As always, it is a good idea to write a testcase in dlls/kernel/tests to
> document this error...

Of course, but I can only write a test if I know what to test/what the
results should be, that's why I'm asking...

bye  Fabi

[Alexandre Julliard]

Fabian Cenedese  writes:
> The found value (info->Data) should only be copied to (buffer) if its length (len)
> is big enough. len is given in bytes (8 for the above call with a buffer of 4 WCHARs).
> But the length of the data (ret) is in WCHAR units. So the comparison here
> is completely wrong. Ok, I could change this to len/sizeof(WCHAR). But that's
> still not enough because of the comparison before about info->Data[ret-1].

len is supposed to be in WCHARs too, most likely the caller is not
passing the correct size.