search

nodejs / http-parser

http request/response parser for c
MIT License
6.32k stars 1.53k forks source link

Fix sizeof(http_parser) assert #510

Closed bnoordhuis closed 4 years ago

bnoordhuis commented 4 years ago

The result should be 32 on both 32 bits and 64 bits architectures because of struct padding.

Fixes: https://github.com/nodejs/http-parser/issues/507

bnoordhuis commented 4 years ago

@indutny Can I get a quick LGTM?

mbakke commented 4 years ago

@bnoordhuis This works on ARMv7, but fails on i686:

$ ./test_g 
http_parser v2.9.4 (0x020904)
sizeof(http_parser) = 28
test_g: test.c:4240: main: Assertion `sizeof(http_parser) == 32' failed.
Aborted
thefloweringash commented 4 years ago

Turns out the comment from stack overflow I read in https://github.com/nodejs/http-parser/issues/507#issuecomment-621403432 was not sufficient. The actual rule for this case is detailed in The Lost Art of Structure Packing.

In general, a struct instance will have the alignment of its widest scalar member

Which is not the same as being aligned to the size of the widest member. Apparently x86 and arm differ on the alignment requirements for 64-bit values, where arm requires 8-byte alignment and x86 requires 4-byte alignment. This particular case is covered by a note about arm and x86 compatibility from intel:

The 8-byte (64-bit) mVar2 results in different layout for TestStruct. This is because ARM requires 8-byte alignment for 64-bit variables like mVar2.

So it seems this test needs to encode the abi requirements.

delroth commented 2 years ago

As mentioned by https://github.com/nodejs/http-parser/pull/510#issuecomment-632054042 the fix which was merged here breaks testing on i686-linux (where sizeof == 28, not 32). It seems like the only net effect was trading i686 compat for armv7 compat.