UB in blob packing (misaligned u32 writes) + signed shift UB in bitset ops

[Pesky01]

Integrating kb_text_shape.h via Zig. 2 UB issues crash immediately under Zig's debug UB traps.

Environment: Windows 11 x86_64, Zig 0.15.2 (mingw), kb_text_shape.h v2.07

Called kbts_ShapePushFontFromMemory(ctx, bytes, len, 0) with Geist or GeistMono variable fonts. No shaping required - crashes during font init.

---

Issue 1: Misaligned u32 store in kbts_PlaceBlob

kbts_PlaceBlob stamps font tables back-to-back into OutData (char*), then casts to kbts_u32* for baked matrices. If total table bytes % 4 != 0, the cast is misaligned.

thread 240276 panic: store of misaligned address 0x1a005cb5676 for type 'unsigned int', which requires 4 byte alignment
C:\Users\pesky\source\repos\cl\third_party\kb\kb_text_shape.h:28215:0: 0x7ff6e0c0b01a in kbts_PlaceBlob (kb.lib)
              LookupSubtableIndexOffsets[RunningLookupIndex] = (kbts_u32)RunningSubtableIndex;

C:\Users\pesky\source\repos\cl\third_party\kb\kb_text_shape.h:25856:0: 0x7ff6e0c00c69 in kbts_ShapePushFontFromMemory (kb.lib)
        Error = kbts_PlaceBlob(Result, &State, Scratch, Output);

C:\Users\pesky\source\repos\cl\src\draw\text_shape.zig:92:53: 0x7ff6e0bab9d6 in pushFont (chess_lab_zcu.obj)
    const font = kbts.c.kbts_ShapePushFontFromMemory(ctx, @ptrCast(@constCast(bytes.ptr)), @intCast(bytes.len), 0);

---

Issue 2: Signed shift UB in bitset ops

Multiple sites do:

Matrix[...] |= 1 << MatrixIndex.BitIndex;

When BitIndex == 31, shifting signed 1 into the sign bit is UB.

This pattern appears in other places as well, where they might be an issue?

thread 240672 panic: left shift of 1 by 31 places cannot be represented in type 'int'
C:\Users\pesky\source\repos\cl\third_party\kb\kb_text_shape.h:27581:0: 0x7ff606fd0b99 in kbts__MarkMatrixCoverage (kb.lib)
            Matrix[MatrixIndex.WordIndex] |= 1 << MatrixIndex.BitIndex;

C:\Users\pesky\source\repos\cl\third_party\kb\kb_text_shape.h:28494:0: 0x7ff606fb0302 in kbts_PlaceBlob (kb.lib)
                      kbts__MarkMatrixCoverage(...);

C:\Users\pesky\source\repos\cl\third_party\kb\kb_text_shape.h:25856:0: 0x7ff606fa0ca9 in kbts_ShapePushFontFromMemory (kb.lib)
        Error = kbts_PlaceBlob(Result, &State, Scratch, Output);

C:\Users\pesky\source\repos\cl\src\draw\text_shape.zig:92:53: 0x7ff606f4b9d6 in pushFont (chess_lab_zcu.obj)
    const font = kbts.c.kbts_ShapePushFontFromMemory(ctx, @ptrCast(@constCast(bytes.ptr)), @intCast(bytes.len), 0);

[JimmyLefevre]

The problem with the misaligned addresses is that OpenType records are tightly packed.

For instance, I'm not sure how to make UBSan happy in this case:

#pragma pack(push, 1)
typedef struct kbts__os2
{
  kbts_u16 Version;
  kbts_s16 AverageCharacterWidth;
  kbts_u16 WeightClass;
  kbts_u16 WidthClass;
  kbts_u16 Type;
  kbts_s16 SubscriptXSize;
  kbts_s16 SubscriptYSize;
  kbts_s16 SubscriptXOffset;
  kbts_s16 SubscriptYOffset;
  kbts_s16 SuperscriptXSize;
  kbts_s16 SuperscriptYSize;
  kbts_s16 SuperscriptXOffset;
  kbts_s16 SuperscriptYOffset;
  kbts_s16 StrikeoutSize;
  kbts_s16 StrikeoutPosition;
  kbts_s16 FamilyClass;
  kbts_u8 Panose[10];
  kbts_u32 UnicodeRange[4]; // <- This is unaligned!
  kbts_u32 VendorId; // <- This is unaligned!
  kbts_u16 Selection;
  kbts_u16 FirstCharacterIndex;
  kbts_u16 LastCharacterIndex;

  // Some version 0 fonts support this, others do not.
  kbts_s16 TypoAscender;
  kbts_s16 TypoDescender;
  kbts_s16 TypoLineGap;
  kbts_u16 WinAscent;
  kbts_u16 WinDescent;

  // If Version >= 1:
  kbts_u32 CodePageRange[2]; // <- This is unaligned!

  // If Version >= 2:
  kbts_s16 Height;
  kbts_s16 CapHeight;
  kbts_u16 DefaultChar;
  kbts_u16 BreakChar;
  kbts_u16 MaxContext;

  // If Version >= 5:
  kbts_u16 LowerOpticalPointSize;
  kbts_u16 UpperOpticalPointSize;
} kbts__os2;

Now, granted, I don't use these values, so a simple solution would be not to touch them, but there's nothing stopping any other u32 anywhere else in the file to be misaligned.
Surely, there's a way to tell UBsan that this is explicitly packed and it's fine?

[Pesky01]

The crash is writing to LookupSubtableIndexOffsets in the output blob, not reading packed font data. Since we control this layout, aligning before the cast fixes it:

OutData = KBTS__ALIGN_POINTER(char, OutData, KBTS_ALIGNOF(kbts_u32));
kbts_u32 *GlyphLookupMatrix = (kbts_u32 *)OutData;

For kbts__os2 that messes with the byteswap reads. Using memcpy internally fixes it, optimized to direct access:

static void kbts__ByteSwapArray16Unchecked(void *Array_, kbts_un Count)
{
    kbts_u8 *Bytes = (kbts_u8 *)Array_;
    KBTS__FOR(It, 0, Count) {
        kbts_u16 V;
        KBTS_MEMCPY(&V, Bytes + It*2, 2);
        V = kbts__ByteSwap16(V);
        KBTS_MEMCPY(Bytes + It*2, &V, 2);
    }
}

For the signed shift, 1 << 31 is UB, just 1u << 31. I forked and put up my modifications for more reference, fixed all the traps I've ran into so far: https://github.com/Pesky01/kb

[david-vanderson]

Hi, I'm integrating kb_text_shape into dvui (zig immediate mode gui - david-vanderson/dvui#666). Thanks so much for the project!

I also ran into these UB issues and am following along here. @Pesky01's changes work for me, but I'm only exercising a small part of them. Let me know if there's anything I can help with.

[JimmyLefevre]

The crash is writing to LookupSubtableIndexOffsets in the output blob, not reading packed font data. Since we control this layout, aligning before the cast fixes it:

OutData = KBTS__ALIGN_POINTER(char, OutData, KBTS_ALIGNOF(kbts_u32));
kbts_u32 *GlyphLookupMatrix = (kbts_u32 *)OutData;

For kbts__os2 that messes with the byteswap reads. Using memcpy internally fixes it, optimized to direct access:

static void kbts__ByteSwapArray16Unchecked(void *Array_, kbts_un Count)
{
kbts_u8 Bytes = (kbts_u8 )Array_;
KBTS__FOR(It, 0, Count) {
kbts_u16 V;
KBTS_MEMCPY(&V, Bytes + It2, 2);
V = kbts__ByteSwap16(V);
KBTS_MEMCPY(Bytes + It2, &V, 2);
}
}

For the signed shift, 1 << 31 is UB, just 1u << 31. I forked and put up my modifications for more reference, fixed all the traps I've ran into so far: https://github.com/Pesky01/kb

This is a good starting point, but I'm afraid it doesn't address the more fundamental issue I pointed out above.

I have fonts that put u32s, and arrays of u32s, at non-multiple-of-4 offsets within the file/their respective tables, and, as far as I know, there is nothing stopping any of the offsets in a font file from being the wrong alignment for the data it points to.

If we want to fully avoid this kind of UB (do you have any target platforms for which this is an issue?), I think we either need our own font representation or we need to use ReadXXXUnaligned whenever we access anything in the font.

With that said, in practice, maybe this is only an issue with u32s, and maybe there are few enough of them that always accessing them explicitly unaligned is not too bad of a patch.

[JimmyLefevre]

This is not exhaustive, but hopefully 9e2b6a2 covers most cases.

[Pesky01]

The crash is writing to LookupSubtableIndexOffsets in the output blob, not reading packed font data. Since we control this layout, aligning before the cast fixes it:
OutData = KBTS__ALIGN_POINTER(char, OutData, KBTS_ALIGNOF(kbts_u32));
kbts_u32 *GlyphLookupMatrix = (kbts_u32 *)OutData;
For kbts__os2 that messes with the byteswap reads. Using memcpy internally fixes it, optimized to direct access:
static void kbts__ByteSwapArray16Unchecked(void *Array_, kbts_un Count)
{
kbts_u8 _Bytes = (kbts_u8 )Array;
KBTS__FOR(It, 0, Count) {
kbts_u16 V;
KBTS_MEMCPY(&V, Bytes + It_2, 2);
V = kbts__ByteSwap16(V);
KBTS_MEMCPY(Bytes + It_2, &V, 2);
}
}
For the signed shift, 1 << 31 is UB, just 1u << 31. I forked and put up my modifications for more reference, fixed all the traps I've ran into so far: https://github.com/Pesky01/kb

This is a good starting point, but I'm afraid it doesn't address the more fundamental issue I pointed out above.

I have fonts that put u32s, and arrays of u32s, at non-multiple-of-4 offsets within the file/their respective tables, and, as far as I know, there is nothing stopping any of the offsets in a font file from being the wrong alignment for the data it points to.

If we want to fully avoid this kind of UB (do you have any target platforms for which this is an issue?), I think we either need our own font representation or we need to use ReadXXXUnaligned whenever we access anything in the font.

With that said, in practice, maybe this is only an issue with u32s, and maybe there are few enough of them that always accessing them explicitly unaligned is not too bad of a patch.

Ahh, yeah doing unaligned read everywhere is a bit annoying. I agree just the u32 handling is sufficient so all even offsets are fine. For u16, you'd need a table at an odd offset (violating checksum) or an odd length field before u16?

[JimmyLefevre]

The odd offset on a u16 did happen in one of my test fonts. Fixing this specific one wasn't a problem, but we'll have to keep introducing unaligned reads if we find annoying fonts that have weird offsets.

In practice, though, the generated code looks fine (on x86_64) when using compile-time-sized memcpy, so that's a relief.

Thanks for nudging me to fix this, by the way. It's not exactly the most fun part of C programming.

[behdad]

If you defined a __attribute__((packed)) struct { uint16_t v; } then the compiler is obliged to make that v be fine with unaligned access.

[david-vanderson]

This is not exhaustive, but hopefully 9e2b6a2 covers most cases.

This fixes at least the issues I've seen so far. Thank you!!

[Pesky01]

Updated to the new v2.09, I only receive the 1st crash still (kbts_ShapePushFontFromMemory), which is just because I have to manually align the embedded font to 4-bytes from the Zig side and it's fine, but the library can still handle that case.

[JimmyLefevre]

If you defined a __attribute__((packed)) struct { uint16_t v; } then the compiler is obliged to make that v be fine with unaligned access.

Good to know, thanks!

Since raw u16 arrays can be unaligned, too, I ended up adding explicit unaligned loads everywhere anyway.