[PATCH v2 03/11] shell32/autocomplete: Handle heap_alloc failure and fix a vulnerability by avoiding the use of sprintf

[Gabriel Ivăncescu]

On Fri, Sep 7, 2018 at 12:02 PM, Huw Davies <huw at codeweavers.com> wrote:
>
> I thought we'd agreed to split this into two patches?
>
>> +                        WCHAR *buf;
>> +                        size_t len = strlenW(hwndText);
>> +                        size_t sz = strlenW(This->quickComplete) + 1;
>> +                        sz += max(len, 2);  /* %s is 2 chars */
>> +
>> +                        /* Replace the first %s directly without using snprintf, to avoid
>> +                           exploits since the format string can be retrieved from the registry */
>> +                        if ((buf = heap_alloc(sz * sizeof(WCHAR))))
>> +                        {
>> +                            WCHAR *qc = This->quickComplete, *dst = buf;
>> +                            do
>> +                            {
>> +                                if (qc[0] == '%' && qc[1] == 's')
>> +                                {
>> +                                    memcpy(dst, hwndText, len * sizeof(WCHAR));
>> +                                    strcpyW(dst + len, qc + 2);
>> +                                    break;
>> +                                }
>> +                                *dst++ = *qc++;
>> +                            } while (qc[-1] != '\0');
>
> Moving the sprintf replacement to a helper function would be good.  Also,
> it should probably cope with unescaping "%%".
>
I must have missed the splitting part. And totally forgot about the
%%, and the max(len, 2) is useless now I'll just get rid of it (and
keep it as simply len).

>
> Err, this suggests two thing to me:
>
> 1. You're not testing properly.
> 2. You're not reviewing you patches properly.
>
> In addition, spaces around the '*' would be nice.
>
Oops, I rushed a bit with that change in and forgot to test because I
thought it was trivial, I was scared to be too slow after replying to
your points. Sorry about that.