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.