stb_sprintf: possible unaligned access

[stefano-zanotti-88]

stb_sprintf: possible unaligned access

In the function 'vsprintfcb': https://github.com/nothings/stb/blob/af1a5bc352164740c1cc1354942b1c6b72eacb8a/stb_sprintf.h#L1104 The cast to write-access 's' is not safe. I think it's safe in practice, but it could theoretically lead to an unaligned access, which is a fault on some architectures.

Following the variable 's', this is the case of the switch we're interested in: https://github.com/nothings/stb/blob/af1a5bc352164740c1cc1354942b1c6b72eacb8a/stb_sprintf.h#L1058 This is where s is set. STBSP__NUMSZ is a multiple of 2, no problems here; 'num' might be misaligned instead. https://github.com/nothings/stb/blob/af1a5bc352164740c1cc1354942b1c6b72eacb8a/stb_sprintf.h#L1088 This is where s is accessed. The decrement is by 2, so no problems here. https://github.com/nothings/stb/blob/af1a5bc352164740c1cc1354942b1c6b72eacb8a/stb_sprintf.h#L1101-L1107

The only problem can come from a misaligned 'num': https://github.com/nothings/stb/blob/af1a5bc352164740c1cc1354942b1c6b72eacb8a/stb_sprintf.h#L571-L585 I'm not sure which alignment requirements are there for the compiler when allocating variables on the stack, but I don't think that a char[] variable is required to be more than 1-byte aligned, at least on some architectures.

A simple solution might be to shuffle the declarations around, to have 'char* s' right before 'num', so as to force 'num' to be aligned like a pointer. However, this doesn't give any real guarantee. A proper solution would be to use __attribute__((aligned(2))) but this is compiler-specific. Or to use _Alignas(uint16_t) but this requires C11 support. So maybe the variable reordering is the best solution, since it is minimally intrusive and the problem should be rare anyway.

I don't have an example of an architecture where the access is actually unaligned. I just got a compiler warning about it and investigated the source code.

Note that the proper alignment of 'num' is also needed for another access: https://github.com/nothings/stb/blob/af1a5bc352164740c1cc1354942b1c6b72eacb8a/stb_sprintf.h#L1824 This access is in the function 'stbsp__real_to_str'; 'out' is a parameter of this function; the 'num' variable mentioned above is the only one to ever get passed into this argument: https://github.com/nothings/stb/blob/af1a5bc352164740c1cc1354942b1c6b72eacb8a/stb_sprintf.h#L720 https://github.com/nothings/stb/blob/af1a5bc352164740c1cc1354942b1c6b72eacb8a/stb_sprintf.h#L755 https://github.com/nothings/stb/blob/af1a5bc352164740c1cc1354942b1c6b72eacb8a/stb_sprintf.h#L824

---

Also related to the alignment issue: other compiler warnings, in cases where the access is guaranteed to be aligned, might be better silenced. For example: https://github.com/nothings/stb/blob/af1a5bc352164740c1cc1354942b1c6b72eacb8a/stb_sprintf.h#L1177-L1187 The first loop ensures the alignment. The access in the second loop could be rewritten like this: *(stbsp__uint32 *)(stbsp__uintptr)bf = 0x20202020; // safe cast (the alignment is ensured by the logic above) which gets rid of the warning, which is: warning: cast increases required alignment of target type [-Wcast-align]

[N-R-K]

A simple solution might be to shuffle the declarations around, to have 'char* s' right before 'num', so as to force 'num' to be aligned like a pointer. However, this doesn't give any real guarantee. A proper solution would be to use __attribute__((aligned(2))) but this is compiler-specific. Or to use _Alignas(uint16_t) but this requires C11 support. So maybe the variable reordering is the best solution, since it is minimally intrusive and the problem should be rare anyway.

I suppose you could also use a union. Something like this:

union { void *align; char num[X]; } aligned_num;
#define num aligned_num.num

If this is an actual problem and not just a theoretical one, then I'd just drop the macro and rename num where needed.

[stefano-zanotti-88]

For now the problem is theoretical. I'm not sure that the union gives any actual guarantee of alignment; just like reordering the variables, I guess it gives no guarantees though it always works in practice.

A bit more problematic is the presence of many "false" warnings, because real issues can then get lost in the noise. These warnings are silenced by the cast to (stbsp__uintptr).

[ypujante]

I don't know if this issue is "theoretical" anymore. I am trying to compile my application (raylib / ImGui) for the web platform using emscripten.

Using stb_snprintf fails (ImGui can use stb_sprintf by defining IMGUI_USE_STB_SPRINTF). If I remove the define (thus using sprintf instead of stb_sprintf) then there is no issue.

Uncaught RuntimeError: Aborted(alignment fault)
    at abort (RE Edit.js:565:10)
    at alignfault (RE Edit.js:332:2)
    at imports.<computed> (RE Edit.js:8723:24)
    at RE Edit.wasm.SAFE_HEAP_STORE_i32_4_4 (RE Edit.wasm:0x1c22fd8)
    at RE Edit.wasm.stbsp_vsprintfcb (RE Edit.wasm:0x1615534)
    at RE Edit.wasm.stbsp_vsnprintf (RE Edit.wasm:0x162f846)
    at RE Edit.wasm.stbsp_snprintf (RE Edit.wasm:0x1630688)

[N-R-K]

I don't know if this issue is "theoretical" anymore

Just to confirm whether your issue is the same as this or not, you can apply the following patch and try again:

@@ -571,7 +571,7 @@ STBSP__PUBLICDEF int STB_SPRINTF_DECORATE(vsprintfcb)(STBSP_SPRINTFCB *callback,
       // handle each replacement
       switch (f[0]) {
          #define STBSP__NUMSZ 512 // big enough for e308 (with commas) or e-307
-         char num[STBSP__NUMSZ];
+         _Alignas(void *) char num[STBSP__NUMSZ];
          char lead[8];
          char tail[8];
          char *s;

If this fixes your problem, then we've found the culprit.

[ypujante]

I am getting:

/Volumes/Development/github/pongasoft/re-mock/external/nothings/stb/stb_sprintf.h:575:10: error: expected expression
  575 |          _Alignas(void *) char num[STBSP__NUMSZ];
      |          ^

It looks like _Alignas is not valid for the compiler. Here is the compiler command:

/usr/local/emsdk/upstream/emscripten/em++  -I/Volumes/Development/github/pongasoft/re-mock/external/nothings/stb -g -std=gnu++17 -fcolor-diagnostics -MD -MT re-mock-build/external/nothings/stb/CMakeFiles/stb.dir/stb.cpp.o -MF re-mock-build/external/nothings/stb/CMakeFiles/stb.dir/stb.cpp.o.d -o re-mock-build/external/nothings/stb/CMakeFiles/stb.dir/stb.cpp.o -c /Volumes/Development/github/pongasoft/re-mock/external/nothings/stb/stb.cpp

[N-R-K]

What about char num[STBSP__NUMSZ] __attribute__((aligned)); ? Does that work?

[ypujante]

It does compile with this syntax, but it produces the same alignment error :(

[stefano-zanotti-88]

I think this is what was meant (mind the "2"): char num[STBSP__NUMSZ] __attribute__((aligned(2)));

Or you could try with this, as a replacement for the current variable definition:

short num16[STBSP__NUMSZ/2];
#define num    ((char*)num16)

which is just an ugly hack for your test, certainly not a proper fix.

[N-R-K]

I think this is what was meant (mind the "2"):

Shouldn't make any difference. Without an "argument" aligned attribute aligns to "maxium useful alignment" (which is likely _Alignof(max_align_t)).

but it produces the same alignment error

I'm not familiar with emscripten but does it produce more meaningful backtrace (with source file and line number) if you compile with -g3 (or -g)?

Also, do you have UBSan available? (compile with -g3 -fsanitize=undefined and see if that works or not).

[ypujante]

@stefano-zanotti-88 none of the 2 "hacks" you suggested fix the issue

@N-R-K Using -g3 -fsanitize=undefined does not seem to generate valid code as it doesn't even run anymore

I am not very familiar with emscripten. I am just following the instructions provided with the raylib library to compile a project for the web platform (the project was originally created and is working fine for the desktop platform). So I am not sure how to troubleshoot further.