Fix undefined behaviour in visual server as reported by ubsan/asan.

[RevoluPowered]

Apparently doesn't apply to other datatypes, I'm unsure why the alignment of float * matters more than an int * alignment.
This code does the same thing its just a bit clearer, and doesn't rely on the code ubsan was complaining was dangerous / had undefined behaviour.

[lawnjelly]

It might be the other code hasn't run, or it wasn't unaligned, or integer isn't alignment sensitive on the platform. This kind of thing (loading from arbitrary vertex formats) I can see could be potentially rather dangerous on e.g. ARM chips where unaligned access can crash. x86 is pretty tolerant of this afaik.

https://stackoverflow.com/questions/63834136/how-to-avoid-unaligned-access-exceptions-with-float-on-cortex-m4

This post suggests a good 'safe' portable way of doing this might be to use memcpy.

I'm not actually sure whether you can just simply use something like this:

float f[3];
memcopy (&f[0], src, sizeof (float) * 3);

or you have to use more like

uint32_t ui[3];
memcopy (&ui[0], src, sizeof (uint32_t) * 3);
const float * pf = (const float *) &ui[0];
// etc

I don't know if your commit is actually any safer, or if it just hides the warning.

[lawnjelly]

Is this correct given that r seems to be 8 bit?

PoolVector<uint8_t>::Read r = p_vertex_data.read();

Use brackets for precedence to make it clear, but I think this does e.g.

const float &y = r[(j * total_elem_size) + offsets[i] + 1];

The +1 is 1 byte, therefore unaligned.

[jordo]

This fix doesn't seem correct? Since r is a byte buffer, wouldn't you want to index?

const float &y = r[j * total_elem_size + offsets[i] + 4]
const float &z = r[j * total_elem_size + offsets[i] + 8]

[akien-mga]

CC @The-O-King

[The-O-King]

I don't think this is connected to one of my changes, but just taking a look, I see that basically all of the code in that function writes to the array similarly, but usually with uint_*t rather than float, and I also don't know if it's any more/less safe to change it to uint32_t for example

The lines that I do touch in this file do something like:

   Vector3 dec = oct_to_tangent(enc, &w[j * 4 + 3]);
   w[j * 4 + 0] = dec.x;
   w[j * 4 + 1] = dec.y;
   w[j * 4 + 2] = dec.z;

Which apparently ubsan is not complaining about? But should this usage also be considered problematic?

[lawnjelly]

Which apparently ubsan is not complaining about? But should this usage also be considered problematic?

You are probably safe if w is a float. Because then the compiler can guarantee it will be aligned.

Afaik the problem is that the OP's version is using reads from an 8 bit size pointer. This suggests the compiler cannot know ahead of time that the reads will be aligned to e.g. 4 byte boundaries, and this may cause undefined behaviour on some systems.

One solution when reading from potentially unaligned sources is to use memcpy instead of casting pointers. memcpy is guaranteed to work on any alignment, and in most cases it seems it will be optimized to something simpler by the compiler where possible (e.g. an unaligned load instruction).

[lawnjelly]

Instead of:

PoolVector<Vector3>::Write w = arr_3d.write();

for (int j = 0; j < p_vertex_len; j++) {
    const float *v = (const float *)&r[j * total_elem_size + offsets[i]];
    w[j] = Vector3(v[0], v[1], v[2]);
}

Attempting to write something without compiling or ubsan, afaik something like this might work:

PoolVector<Vector3>::Write w = arr_3d.write();

// or possibly ptr(), but get a pointer to start of r array
const uint8_t * src = &r[0];

// add the constant for the start of the data into the structure
src += offsets[i];

for (int j = 0; j < p_vertex_len; j++) {
    memcpy(&w[j], src, 3 * sizeof (float));
    src += total_elem_size;
}

You might have to adjust the instructions to get the pointers to the godot arrays, I'm not super familiar without the reference.

This is just what I think it might be complaining about, and I don't have access to the ubsan error lines etc.

[RevoluPowered]

@lawnjelly here is the ubsan log

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior servers/visual_server.cpp:1433:23 in 
servers/visual_server.cpp:1433:29: runtime error: load of misaligned address 0x7fa140252022 for type 'const float', which requires 4 byte alignment
0x7fa140252022: note: pointer points here
 68 e6  eb c2 cd cc cc 3f 00 00  d0 40 00 00 68 e6 eb c2  00 00 00 00 00 00 d0 40  00 00 02 80 b8 c2
              ^ 
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior servers/visual_server.cpp:1433:29 in 
servers/visual_server.cpp:1433:35: runtime error: load of misaligned address 0x7fa140252026 for type 'const float', which requires 4 byte alignment
0x7fa140252026: note: pointer points here
 cd cc cc 3f 00 00  d0 40 00 00 68 e6 eb c2  00 00 00 00 00 00 d0 40  00 00 02 80 b8 c2 00 00  00 00```

[lawnjelly]

Yeah looks like it is loading on a 16 bit boundary from the address 0x7fa140252026 (it should be 0, 4, 8, c, etc for 4 byte aligned I think).

[RevoluPowered]

Yeah looks like it is loading on a 16 bit boundary from the address 0x7fa140252026 (it should be 0, 4, 8, c, etc for 4 byte aligned I think).

is there somewhere I can read online so I understand this better? :D

[lawnjelly]

is there somewhere I can read online so I understand this better? :D

Oo now there's a tough one! 😄

If you do any SIMD programming this will make you pay much more attention to alignment, because there are aligned and unaligned load functions, and the aligned can be a lot faster in some situations, and using the wrong one can lead to crash (even on x86). On a lot of ARM chips an unaligned load / store of just e.g. a float can cause a crash as far as I remember.

https://en.wikipedia.org/wiki/Data_structure_alignment

Agner Fog also has lots of great low level info:
https://www.agner.org/optimize/

Aside from crashes, another great reason to pay attention to alignment is to make good use of your cache lines. The CPU doesn't just read 1 byte from memory it reads a cache line at a time, usually 64 bytes. If you try and read something that straddles this cache line, even for a 16 bit value, you are loading in a bunch of memory that doesn't need to be there. And memory access is one of the biggest bottlenecks these days.

I'm far from a low level expert myself, I just dabble every now and then, and used to do assembly in the long distant past. There are also some great big cheeses of low level on twitter like e.g. fabian giesen https://twitter.com/rygorous .

[jordo]

This fix, likely will not resolve the issue, and is likely wrong. (the offsets for floating point indices into the byte buffer are wrong).

Ubsan is runtime instrumentation. Loads are instrumented through the ubsan runtime. This has nothing to do with compiler.

There is likely a bug in whats being passed in here ( total_elem_size or offsets[i] ). If those values are correct, all the float loads are going to be 4-byte aligned which is what's expected to happen. If either one of these are wrong and index into the byte array isn't a multiple of 4, we have a bug, and we're just by chance catching it as an unaligned floating point load (thank you ubsan).

UBSan is picking up a bug here in either total_elem_size or offsets[i]. Addition of these two need to be a multiple of 4. The existing code will run fine, provided total_elem_size and the value read in offsets[i] are correct

[The-O-King]

If this is a 4-byte alignment issue then perhaps it's going to be resolved with this PR that I made (#51376) as a previous implementation of Octahedral Normal compression supported vertex attributes that were 2 bytes in size

Although I haven't tested this yet, so I can't say for sure if this would fix it, as this issue seems to be occurring within the vertex position writes, which happen before the normals/tangents ever get written. And the offsets for vertex positions was not modified by the octahedral compression implementation, so I'm not actually sure if these two things are connected.

It could have something to do with the split_stream implementation (#46574) as well, but also haven't gotten a chance to check on that yet

@RevoluPowered have you gotten a chance to test the alignment PR?

[RevoluPowered]

If this is a 4-byte alignment issue then perhaps it's going to be resolved with this PR that I made (#51376) as a previous implementation of Octahedral Normal compression supported vertex attributes that were 2 bytes in size

Although I haven't tested this yet, so I can't say for sure if this would fix it, as this issue seems to be occurring within the vertex position writes, which happen before the normals/tangents ever get written. And the offsets for vertex positions was not modified by the octahedral compression implementation, so I'm not actually sure if these two things are connected.

It could have something to do with the split_stream implementation (#46574) as well, but also haven't gotten a chance to check on that yet

@RevoluPowered have you gotten a chance to test the alignment PR?

Tested and it works fine :)

Submitted a proposal from all the discussion here so we can decide what to do going forward : please take a look
godotengine/godot-proposals#3151

[RevoluPowered]

Closing :) godotengine/godot-proposals#3151

We are going to fix the actual problem: the invalid data being imported.