[PATCH v2 0/1] MR4930: wineps.drv: Avoid invalid unaligned accesses.
While armv7 generally tolerates unaligned loads/stores in most cases, the compiler is free to use the ldrd/strd instructions, for loading/storing two consecutive 32 bit registers, and this requires the destination to be aligned to a 4 byte boundary. When packing a number of variable length structures, make sure that each actual struct gets aligned at the right address boundary. This fixes crashes in DllMain of wineps.drv, when built for armv7, since 351e58dc2d0aafe19294cbeaed9cd30ae965d591. Signed-off-by: Martin Storsjö <martin(a)martin.st> -- v2: wineps.drv: Avoid invalid unaligned accesses. https://gitlab.winehq.org/wine/wine/-/merge_requests/4930
From: Martin Storsjö <martin(a)martin.st> While armv7 generally tolerates unaligned loads/stores in most cases, the compiler is free to use the ldrd/strd instructions, for loading/storing two consecutive 32 bit registers, and this requires the destination to be aligned to a 4 byte boundary. When packing a number of variable length structures, make sure that each actual struct gets aligned at the right address boundary. This fixes crashes in DllMain of wineps.drv, when built for armv7, since 351e58dc2d0aafe19294cbeaed9cd30ae965d591. Signed-off-by: Martin Storsjö <martin(a)martin.st> --- dlls/wineps.drv/init.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/dlls/wineps.drv/init.c b/dlls/wineps.drv/init.c index 8537ba287d1..8d39a310d4a 100644 --- a/dlls/wineps.drv/init.c +++ b/dlls/wineps.drv/init.c @@ -133,6 +133,9 @@ static BOOL import_ntf_from_reg(void) return ret; } +#define ntf_strlen(str) ((strlen(str) + 4) & ~3) +#define ntf_wcslen(str) ((wcslen(str) + 2) & ~1) + static BOOL convert_afm_to_ntf(void) { int i, count, size, off, metrics_size; @@ -176,8 +179,8 @@ static BOOL convert_afm_to_ntf(void) list = (void *)(data + header->glyph_set_off + sizeof(*list) * count); list->name_off = off + sizeof(*glyph_set); - list->size = sizeof(*glyph_set) + strlen(glyph_set_name) + 1 + sizeof(*cp) + - sizeof(short) * afmle->afm->NumofMetrics; + list->size = sizeof(*glyph_set) + ntf_strlen(glyph_set_name) + sizeof(*cp) + + sizeof(short) * ((afmle->afm->NumofMetrics + 1) & ~1); list->off = off; size += list->size; new_data = realloc(data, size); @@ -196,7 +199,7 @@ static BOOL convert_afm_to_ntf(void) glyph_set->name_off = sizeof(*glyph_set); glyph_set->glyph_count = afmle->afm->NumofMetrics; glyph_set->cp_count = 1; - glyph_set->cp_off = glyph_set->name_off + strlen(glyph_set_name) + 1; + glyph_set->cp_off = glyph_set->name_off + ntf_strlen(glyph_set_name); glyph_set->glyph_set_off = glyph_set->cp_off + sizeof(*cp); strcpy(data + off + glyph_set->name_off, glyph_set_name); cp = (void *)(data + off + glyph_set->cp_off); @@ -206,11 +209,11 @@ static BOOL convert_afm_to_ntf(void) off = size; metrics_size = sizeof(IFIMETRICS) + - (wcslen(afmle->afm->FamilyName) + 1) * sizeof(WCHAR); + ntf_wcslen(afmle->afm->FamilyName) * sizeof(WCHAR); list = (void *)(data + header->font_mtx_off + sizeof(*list) * count); list->name_off = off + sizeof(*font_mtx); - list->size = sizeof(*font_mtx) + strlen(afmle->afm->FontName) + 1 + - strlen(glyph_set_name) + 1 + metrics_size + + list->size = sizeof(*font_mtx) + ntf_strlen(afmle->afm->FontName) + + ntf_strlen(glyph_set_name) + metrics_size + (afmle->afm->IsFixedPitch ? 0 : sizeof(*width_range) * afmle->afm->NumofMetrics); list->off = off; size += list->size; @@ -227,9 +230,9 @@ static BOOL convert_afm_to_ntf(void) font_mtx = (void *)(data + off); font_mtx->size = size - off; font_mtx->name_off = sizeof(*font_mtx); - font_mtx->glyph_set_name_off = font_mtx->name_off + strlen(afmle->afm->FontName) + 1; + font_mtx->glyph_set_name_off = font_mtx->name_off + ntf_strlen(afmle->afm->FontName); font_mtx->glyph_count = afmle->afm->NumofMetrics; - font_mtx->metrics_off = font_mtx->glyph_set_name_off + strlen(glyph_set_name) + 1; + font_mtx->metrics_off = font_mtx->glyph_set_name_off + ntf_strlen(glyph_set_name); font_mtx->width_count = afmle->afm->IsFixedPitch ? 0 : afmle->afm->NumofMetrics; font_mtx->width_off = font_mtx->metrics_off + metrics_size; font_mtx->def_width = afmle->afm->Metrics[0].WX; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4930
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=142340 Your paranoid android. === debian11b (64 bit WoW report) === mf: Unhandled exception: divide by zero in 64-bit code (0x0000000042e22c).
On Wed Jan 24 18:51:33 2024 +0000, Martin Storsjö wrote:
Thanks, that sounds like a reasonable solution. I would expect all the involved structs to be a multiple of 4 already, and padding the strings that way should be enough indeed. And wrapping up the strlens in such a macro sounds like a neat way to fix it. And 4 byte alignment should be fine (I didn’t check what the structs contain, but that should be enough for the ldrd/strd instructions on arm at least). This mostly worked fine - I could encapsulate most of these into `ntf_strlen` and `ntf_wcslen`, but there was also one case of `sizeof(short) * afmle->afm->NumofMetrics` where I introduced a direct rounding upwards there inline.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4930#note_58878
Piotr Caban (@piotr) commented about dlls/wineps.drv/init.c:
return ret; }
+#define ntf_strlen(str) ((strlen(str) + 4) & ~3) +#define ntf_wcslen(str) ((wcslen(str) + 2) & ~1)
The patch looks good for me now. I'm not sure if it's better but how about changing the helpers to: ```suggestion:-1+0 #define DWORD_ALIGN(x) ((x) + 3) & ~3) #define ntf_strsize(str) DWORD_ALIGN(strlen(str) + 1) #define ntf_wcssize(str) DWORD_ALIGN((wcslen(str) + 1) * sizeof(WCHAR)) ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4930#note_58950
participants (3)
-
Martin Storsjö -
Marvin -
Piotr Caban (＠piotr)