[PATCH 0/1] MR4930: wineps.drv: Avoid invalid unaligned accesses.

[Martin Storsjö]

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>

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4930

[PATCH 1/1] wineps.drv: Avoid invalid unaligned accesses.

[Martin Storsjö]

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 | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/dlls/wineps.drv/init.c b/dlls/wineps.drv/init.c
index 8537ba287d1..eabdc5546ae 100644
--- a/dlls/wineps.drv/init.c
+++ b/dlls/wineps.drv/init.c
@@ -36,6 +36,9 @@
 
 WINE_DEFAULT_DEBUG_CHANNEL(psdrv);
 
+#define ALIGNED_LENGTH(_Len, _Align) (((_Len)+(_Align))&~(_Align))
+#define ALIGN_LENGTH(_Len, _Align) _Len = ALIGNED_LENGTH(_Len, _Align)
+
 static const PSDRV_DEVMODE DefaultDevmode =
 {
   { /* dmPublic */
@@ -155,14 +158,16 @@ static BOOL convert_afm_to_ntf(void)
         for(afmle = family->afmlist; afmle; afmle = afmle->next)
             count++;
     }
-    size = sizeof(*header) + sizeof(*list) * count * 2;
+    size = ALIGNED_LENGTH(sizeof(*header), TYPE_ALIGNMENT(struct list_entry) - 1) +
+           sizeof(*list) * count * 2;
+    ALIGN_LENGTH(size, TYPE_ALIGNMENT(struct glyph_set) - 1);
     data = calloc(size, 1);
 
     if (!data)
         return FALSE;
     header = (void *)data;
     header->glyph_set_count = count;
-    header->glyph_set_off = sizeof(*header);
+    header->glyph_set_off = ALIGNED_LENGTH(sizeof(*header), TYPE_ALIGNMENT(struct list_entry) - 1);
     header->font_mtx_count = count;
     header->font_mtx_off = header->glyph_set_off + sizeof(*list) * count;
 
@@ -180,6 +185,7 @@ static BOOL convert_afm_to_ntf(void)
                 sizeof(short) * afmle->afm->NumofMetrics;
             list->off = off;
             size += list->size;
+            ALIGN_LENGTH(size, TYPE_ALIGNMENT(struct font_mtx) - 1);
             new_data = realloc(data, size);
             if (!new_data)
             {
@@ -209,11 +215,12 @@ static BOOL convert_afm_to_ntf(void)
                 (wcslen(afmle->afm->FamilyName) + 1) * 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 = ALIGNED_LENGTH(sizeof(*font_mtx) + strlen(afmle->afm->FontName) + 1 +
+                strlen(glyph_set_name) + 1, TYPE_ALIGNMENT(IFIMETRICS) - 1) + metrics_size +
                 (afmle->afm->IsFixedPitch ? 0 : sizeof(*width_range) * afmle->afm->NumofMetrics);
             list->off = off;
             size += list->size;
+            ALIGN_LENGTH(size, TYPE_ALIGNMENT(struct glyph_set) - 1);
             new_data = realloc(data, size);
             if (!new_data)
             {
@@ -229,7 +236,8 @@ static BOOL convert_afm_to_ntf(void)
             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_count = afmle->afm->NumofMetrics;
-            font_mtx->metrics_off = font_mtx->glyph_set_name_off + strlen(glyph_set_name) + 1;
+            font_mtx->metrics_off = ALIGNED_LENGTH(font_mtx->glyph_set_name_off + strlen(glyph_set_name) + 1,
+                                                   TYPE_ALIGNMENT(IFIMETRICS) - 1);
             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

[PATCH 1/1] wineps.drv: Avoid invalid unaligned accesses.

[Marvin]

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=142327

Your paranoid android.


=== debian11b (64 bit WoW report) ===

mf:
Unhandled exception: divide by zero in 64-bit code (0x0000000042e22c).

[Martin Storsjö]

This is admittedly not very pretty - it's quite complicated and brittle to know exactly which type to align against for each case when bumping some size/offset forward. I guess one also could align everything to `max_align_t` or something along those lines, for simplicity.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4930#note_58823

[Piotr Caban (＠piotr)]

This is a native format that we're trying to mimic (even so we have some Wine-specific extensions now). I have analyzed some native NTF files and it looks like we should pad all strings to 4-byte boundary. It would be probably best to introduce something like `ntf_strlen` helper that returns `(strlen(str) + 4) & ~3` and use it instead of `strlen`. I'm expecting this to solve all the alignment related problems but I haven't tested it.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4930#note_58857

[Martin Storsjö]

On Wed Jan 24 18:51:33 2024 +0000, Piotr Caban wrote:
> This is a native format that we're trying to mimic (even so we have some
> Wine-specific extensions now). I have analyzed some native NTF files and
> it looks like we should pad all strings to 4-byte boundary. It would be
> probably best to introduce something like `ntf_strlen` helper that
> returns `(strlen(str) + 4) & ~3` and use it instead of `strlen`. I'm
> expecting this to solve all the alignment related problems but I didn't
> test it.
> 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).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/4930#note_58865