On Fri, Sep 7, 2018 at 12:02 PM, Huw Davies <huw(a)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.