183617 2018-03-13 15:47:23 -0700 [ARM] Build fails due to invalid cast on 32-bit targets 2018-04-10 20:26:44 -0700 1 1 1 Unclassified WebKit bmalloc Other Unspecified Unspecified RESOLVED DUPLICATE 183508 P2 Normal --- 1 aperez aperez ews-watchlist ggaren mark.lam mcatanzaro msaboff saam ysuzuki zan oldest_to_newest 1406269 0 aperez 2018-03-13 15:47:23 -0700 While trying to update the WebKitGTK+ packaging in Buildroot to the recent 2.20.0 release I've found that the following prevents from building for 32-bit ARM (more precisely, this was targeting ARMv7): In file included from ../../bmalloc/bmalloc/HeapKind.h:30:0, from ../../bmalloc/bmalloc/ObjectType.h:30, from ../../bmalloc/bmalloc/BumpAllocator.h:31, from ../../bmalloc/bmalloc/Allocator.h:30, from ../../bmalloc/bmalloc/Cache.h:29, from ../../bmalloc/bmalloc/bmalloc.h:29, from FastMalloc.cpp:246: ../../bmalloc/bmalloc/Gigacage.h: In function ‘Gigacage::BasePtrs& Gigacage::basePtrs()’: ../../bmalloc/bmalloc/Gigacage.h:136:59: warning: cast from ‘char*’ to ‘Gigacage::BasePtrs*’ increases required alignment of target type [-Wcast-align] return *reinterpret_cast<BasePtrs*>(g_gigacageBasePtrs); ^ 1406299 1 335746 aperez 2018-03-13 17:04:43 -0700 Created attachment 335746 Patch 1406300 2 ews-watchlist 2018-03-13 17:05:58 -0700 Attachment 335746 did not pass style-queue: ERROR: Source/bmalloc/ChangeLog:3: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: invalid cast [changelog/unwantedsecurityterms] [3] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 1406308 3 335746 mcatanzaro 2018-03-13 17:27:01 -0700 Comment on attachment 335746 Patch Surely this does not actually fix the alignment problem...? 1406314 4 aperez 2018-03-13 17:47:27 -0700 (In reply to Michael Catanzaro from comment #3) > Comment on attachment 335746 [details] > Patch > > Surely this does not actually fix the alignment problem...? I had first: return *reinterpret_cast<BasePtrs*>(static_cast<void*>(g_gigacageBasePtrs)); but the simpler on seemed to work. I'll do one clean build, clearing ccache, as today Carlos Lopez has found a footgun in Buildroot's ccache handling and it could be that the build succeeded when it shouldn't. I'll remove the cq+ in the meanwhile. 1406327 5 335746 ews-watchlist 2018-03-13 18:19:52 -0700 Comment on attachment 335746 Patch Attachment 335746 did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/6943489 New failing tests: stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate 1406569 6 aperez 2018-03-14 17:54:23 -0700 (In reply to Adrian Perez from comment #4) > (In reply to Michael Catanzaro from comment #3) > > Comment on attachment 335746 [details] > > Patch > > > > Surely this does not actually fix the alignment problem...? > > I had first: > > return > *reinterpret_cast<BasePtrs*>(static_cast<void*>(g_gigacageBasePtrs)); > > but the simpler on seemed to work. I'll do one clean build, clearing > ccache, as today Carlos Lopez has found a footgun in Buildroot's ccache > handling and it could be that the build succeeded when it shouldn't. > I'll remove the cq+ in the meanwhile. Yep, it was a bad ccache case (╯°□°）╯︵ ┻━┻ I'l re-upload the good version tomorrow. 1413161 7 mcatanzaro 2018-04-09 20:49:14 -0700 *Poke.* 1413176 8 mark.lam 2018-04-09 21:37:55 -0700 (In reply to Adrian Perez from comment #0) > While trying to update the WebKitGTK+ packaging in Buildroot to the > recent 2.20.0 release I've found that the following prevents from > building for 32-bit ARM (more precisely, this was targeting ARMv7): > > In file included from ../../bmalloc/bmalloc/HeapKind.h:30:0, > from ../../bmalloc/bmalloc/ObjectType.h:30, > from ../../bmalloc/bmalloc/BumpAllocator.h:31, > from ../../bmalloc/bmalloc/Allocator.h:30, > from ../../bmalloc/bmalloc/Cache.h:29, > from ../../bmalloc/bmalloc/bmalloc.h:29, > from FastMalloc.cpp:246: > ../../bmalloc/bmalloc/Gigacage.h: In function ‘Gigacage::BasePtrs& > Gigacage::basePtrs()’: > ../../bmalloc/bmalloc/Gigacage.h:136:59: warning: cast from ‘char*’ to > ‘Gigacage::BasePtrs*’ increases required alignment of target type > [-Wcast-align] > return *reinterpret_cast<BasePtrs*>(g_gigacageBasePtrs); > ^ Based on this, I'm quite sure your fix is incorrect. The definition of g_gigacageBasePtrs is as follows: extern "C" BEXPORT char g_gigacageBasePtrs[GIGACAGE_BASE_PTRS_SIZE] __attribute__((aligned(GIGACAGE_BASE_PTRS_SIZE))); where ... #define GIGACAGE_BASE_PTRS_SIZE 4096 and ... struct BasePtrs { void* primitive; void* jsValue; void* string; }; That is: g_gigacageBasePtrs is expected to be aligned on a 4096 boundary. Why would it increase alignment of the target type if we cast it to a BasePtrs* which points to a 12 byte type. Hence, your real issue here is that your allocation of g_gigacageBasePtrs is not aligned on 4096 bytes. Even if you silence the compiler warning, you're not fixing the real bug, and will get unexpected failures later during code execution. 1413178 9 mark.lam 2018-04-09 21:43:27 -0700 (In reply to Mark Lam from comment #8) > Even if you silence the compiler warning, you're not fixing the real bug, > and will get unexpected failures later during code execution. See protectGigacageBasePtrs() and unprotectGigacageBasePtrs(). The code expects to be able to mprotect g_gigacageBasePtrs. If g_gigacageBasePtrs is not aligned on a page boundary, then you'll not get the behavior the code expects. 1413207 10 ysuzuki 2018-04-09 22:59:40 -0700 (In reply to Mark Lam from comment #8) > (In reply to Adrian Perez from comment #0) > > While trying to update the WebKitGTK+ packaging in Buildroot to the > > recent 2.20.0 release I've found that the following prevents from > > building for 32-bit ARM (more precisely, this was targeting ARMv7): > > > > In file included from ../../bmalloc/bmalloc/HeapKind.h:30:0, > > from ../../bmalloc/bmalloc/ObjectType.h:30, > > from ../../bmalloc/bmalloc/BumpAllocator.h:31, > > from ../../bmalloc/bmalloc/Allocator.h:30, > > from ../../bmalloc/bmalloc/Cache.h:29, > > from ../../bmalloc/bmalloc/bmalloc.h:29, > > from FastMalloc.cpp:246: > > ../../bmalloc/bmalloc/Gigacage.h: In function ‘Gigacage::BasePtrs& > > Gigacage::basePtrs()’: > > ../../bmalloc/bmalloc/Gigacage.h:136:59: warning: cast from ‘char*’ to > > ‘Gigacage::BasePtrs*’ increases required alignment of target type > > [-Wcast-align] > > return *reinterpret_cast<BasePtrs*>(g_gigacageBasePtrs); > > ^ > > Based on this, I'm quite sure your fix is incorrect. The definition of > g_gigacageBasePtrs is as follows: > > extern "C" BEXPORT char g_gigacageBasePtrs[GIGACAGE_BASE_PTRS_SIZE] > __attribute__((aligned(GIGACAGE_BASE_PTRS_SIZE))); > > where ... > > #define GIGACAGE_BASE_PTRS_SIZE 4096 > > and ... > > struct BasePtrs { > void* primitive; > void* jsValue; > void* string; > }; > > That is: g_gigacageBasePtrs is expected to be aligned on a 4096 boundary. > Why would it increase alignment of the target type if we cast it to a > BasePtrs* which points to a 12 byte type. Hence, your real issue here is > that your allocation of g_gigacageBasePtrs is not aligned on 4096 bytes. > Even if you silence the compiler warning, you're not fixing the real bug, > and will get unexpected failures later during code execution. Is it because g_gigacageBasePtrs are `char` array? I think the compiler says alignment of `char` and `BasePtrs` are different. 1413473 11 aperez 2018-04-10 17:22:45 -0700 In case it helps: I have made a build with a more recent WebKit source and seems to work fine after r230380 was landed by Yusuke for bug #18350. So maybe we can close this one as duplicate. I had for a bit a patch with a double cast —to “void*”, then to “BasePtrs*”— sitting on my lap for a bit, but I didn't manage to wrap my head around why that works, and therefore decided to not submit it. 1413518 12 mcatanzaro 2018-04-10 18:53:40 -0700 Sounds like this is fixed, if the build is no longer failing and the warning is gone? 1413539 13 mark.lam 2018-04-10 20:26:44 -0700 This was actually resolved by Yusuke's alignas patch which gave g_gigacageBasePtrs the alignment it needs. *** This bug has been marked as a duplicate of bug 183508 *** 335746 2018-03-13 17:04:43 -0700 2018-04-09 21:38:04 -0700 Patch bug-183617-20180314020438.patch text/plain 1371 aperez U3VidmVyc2lvbiBSZXZpc2lvbjogMjI5NTg4CmRpZmYgLS1naXQgYS9Tb3VyY2UvYm1hbGxvYy9D aGFuZ2VMb2cgYi9Tb3VyY2UvYm1hbGxvYy9DaGFuZ2VMb2cKaW5kZXggMTNjYWQ2MWZjMDBkOTM3 Nzc0NTcxZDAxNDc0MTc1ZGI0ODZiZWE2Yi4uNzQxZTExYjMzNjBmYTkzZGQ1NzVkMmFiNzgyYWQ2 ZjllNjU3YjA0ZiAxMDA2NDQKLS0tIGEvU291cmNlL2JtYWxsb2MvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9ibWFsbG9jL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDE4LTAzLTEzICBBZHJp YW4gUGVyZXogZGUgQ2FzdHJvICA8YXBlcmV6QGlnYWxpYS5jb20+CisKKyAgICAgICAgW0FSTV0g QnVpbGQgZmFpbHMgZHVlIHRvIGludmFsaWQgY2FzdCBvbiAzMi1iaXQgdGFyZ2V0cworICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTgzNjE3CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQ2FzdGluZyB0byBhIHJl ZmVyZW5jZSB0eXBlIGRpcmVjdGx5IGlzIGFsbG93ZWQgaW4gQysrLCBhbmQgaXQgYmV0dGVyCisg ICAgICAgIHNlZW1zIHRvIGJldHRlciByZWZsZWN0IHRoZSBpbnRlbnRpb24gb2YgdGhlIGNhc3Qu CisKKyAgICAgICAgKiBibWFsbG9jL0dpZ2FjYWdlLmg6CisgICAgICAgIChHaWdhY2FnZTo6YmFz ZVB0cnMpOiBEaXJlY3RseSByZWludGVycHJldF9jYXN0PD4gdG8gdGhlIHJlZmVyZW5jZSB0eXBl LgorCiAyMDE4LTAzLTEwICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CiAKICAgICAg ICAgUGVyUHJvY2Vzczw+IHNob3VsZCBiZSBzYWZlIGJ5IGRlZmF1bHQKZGlmZiAtLWdpdCBhL1Nv dXJjZS9ibWFsbG9jL2JtYWxsb2MvR2lnYWNhZ2UuaCBiL1NvdXJjZS9ibWFsbG9jL2JtYWxsb2Mv R2lnYWNhZ2UuaAppbmRleCBiZWYzYWY1ZDUzYWQ1OGQ1YjlhMmM3ZThlMzhmNzVkNWRkMGU2OGVl Li5jNjhiZjk2YTA1NWRiNTNhZGQ5MTI0YTAwMWFhNzI0YzhiMjU3MGIwIDEwMDY0NAotLS0gYS9T b3VyY2UvYm1hbGxvYy9ibWFsbG9jL0dpZ2FjYWdlLmgKKysrIGIvU291cmNlL2JtYWxsb2MvYm1h bGxvYy9HaWdhY2FnZS5oCkBAIC0xMzMsNyArMTMzLDcgQEAgQklOTElORSB2b2lkKiYgYmFzZVB0 cihCYXNlUHRycyYgYmFzZVB0cnMsIEtpbmQga2luZCkKIAogQklOTElORSBCYXNlUHRycyYgYmFz ZVB0cnMoKQogewotICAgIHJldHVybiAqcmVpbnRlcnByZXRfY2FzdDxCYXNlUHRycyo+KGdfZ2ln YWNhZ2VCYXNlUHRycyk7CisgICAgcmV0dXJuIHJlaW50ZXJwcmV0X2Nhc3Q8QmFzZVB0cnMmPihn X2dpZ2FjYWdlQmFzZVB0cnMpOwogfQogCiBCSU5MSU5FIHZvaWQqJiBiYXNlUHRyKEtpbmQga2lu ZCkK