On Thu Sep 22 20:13:56 2022 +0000, **** wrote:
Paul Gofman replied on the mailing list: ``` On 9/22/22 14:16, Stefan Rentsch (@restet) wrote:
On Thu Sep 22 12:12:12 2022 +0000, Piotr Caban wrote:
I agree with Paul, it's not something we want to add. You are right that it is not necessary to replace all stack buffers. In many cases the bounds are handled properly. However, while fixing stack overflows in shell32/shlexec.c, I repeatedly used same malloc & strlen functions for manual length calculations, which is error-prone. Furthermore, the final string length can sometimes not be easily computed (e.g. `SHELL_ArgifyW`). That is why I came to the conclusion that moving basic operations (i.e. string concat or string formatting) to helper functions would be the safest and most convenient approach. In performance-critical cases the container should be preallocated with the expected final size (where applicable) to avoid realloc overhead. If there is a common pattern in shell32 maybe it worth considering some helper functions specific to shell32 module? Again, it is unlikely beneficial to have a generic custom string container implementation, but maybe moving a repeated check / copy pattern into a helper function may be fine if looks good.
What exactly makes a generic implementation *not* beneficial in long-term? Currently there are least the following places that use dynamic string containers in private helper functions:
* atl/registrar.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/atl/registrar.c#L6... * jscript/json.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/jscript/json.c#L36... * mshtml/range.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/mshtml/range.c#L35... * ntdll/unix/env.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/ntdll/unix/env.c#L... (dynamic string, no container) * vbscript/vbregexp.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/vbscript/vbregexp.... * wineps.drv/type1.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/wineps.drv/type1.c... * winex11.drv/clipboard.c: https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/dlls/winex11.drv/clipbo... (dynamic string, no container) Each implementation seems to be written from scratch, introducing code duplication. I could copy & paste a few functions for shell32 to ensure safe concat and bounds checks, which however again introduces more code to maintain, if that's the preferred practice. EDIT: I am open to suggestions. Please let me know if I can do something. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/910#note_9207