Re: [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  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:
> - You're not testing properly.
> - 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.