Bug 183617 – [ARM] Build fails due to invalid cast on 32-bit targets

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: bmalloc

[Adrian Perez]

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);
                                                             ^

[Adrian Perez]

Created attachment 335746 [details]
Patch

[EWS Watchlist]

Attachment 335746 [details] 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.

[Michael Catanzaro]

Comment on attachment 335746 [details]
Patch

Surely this does not actually fix the alignment problem...?

[Adrian Perez]

(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.

[EWS Watchlist]

Comment on attachment 335746 [details]
Patch

Attachment 335746 [details] 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

[Adrian Perez]

(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.

[Michael Catanzaro]

*Poke.*

[Mark Lam]

(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.

[Mark Lam]

(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.

[Yusuke Suzuki]

(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.

[Adrian Perez]

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.

[Michael Catanzaro]

Sounds like this is fixed, if the build is no longer failing and the warning is gone?

[Mark Lam]

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 ***