Memory overrun due to alignment issues when cross-compiling mingw on linux

[Orgad Shaneh]

Hi,

When I cross-compile nginx for Windows on linux host using mxe, NGX_ALIGNMENT is set to 16. This is set in auto/os/conf:

case "$NGX_MACHINE" in
...
    *)
        have=NGX_ALIGNMENT value=16 . auto/define
        NGX_MACH_CACHE_LINE=32
    ;;
esac

But the pool is allocated using plain malloc (on Windows target), which is *not* aligned to 16 bytes (it's aligned to 4), so pool->d.end is 4-byte aligned.

Now, in ngx_palloc_small, the following code causes integer underflow, and the comparison doesn't work as expected:

        m = p->d.last;

        if (align) {
            m = ngx_align_ptr(m, NGX_ALIGNMENT);
        }

        if ((size_t) (p->d.end - m) >= size) { /* <------- This condition */
            p->d.last = m + size;

            return m;
        }

Since m is aligned, and end is not, end - m can be a negative number. Then it is cast to size_t, which is unsigned, and so it just keeps writing past the memory pool, and overruns the next pool.

A minimal patch that solves this issue is:

src/core/ngx_palloc.c

@@ -160 +160 @@
             m = ngx_align_ptr(m, NGX_ALIGNMENT);
         }
 
-        if ((size_t) (p->d.end - m) >= size) {
+        if (m + size <= p->d.end) {
             p->d.last = m + size;
 
             return m;

But it would be better to assign the right alignment for Windows, even when cross-building. And also ensure that end is aligned correctly.

I also noticed that the default alignment when not set is sizeof(long), which is wrong for Win64. It should be sizeof(intptr_t).

Build command:

CC=i686-w64-mingw32.static-gcc ./auto/configure --crossbuild=win32 --without-http_rewrite_module --without-http_gzip_module

$ grep -1 ALIGN objs/ngx_auto_config.h

#ifndef NGX_ALIGNMENT
#define NGX_ALIGNMENT  16
#endif

[Maxim Dounin]

Thanks for reporting this.

Crossbuilds are not really supported. The --crossbuild configure option was used a while ago for compilation of Windows binaries on Unix systems using Wine, mostly for compile-testing, though it is not really supported, and intentionally not documented neither in ./configure --help nor in the ​documentation. Nevertheless, this issue probably worth fixing.

Note though that the condition in ngx_palloc_small() is perfectly correct, but assumes pointers properly aligned at least to NGX_ALIGNMENT (see #686). Further, the suggested patch looks wrong, as m + size is not guaranteed to exist, and might overflow, leading to incorrect results even with correctly aligned pointers.

As I see it, there are two problems here:

1. Alignment requirements are not correctly set, since ./configure does not know the machine architecture and assumes strict alignment requirements.

2. Windows provides no standard interfaces for aligned allocations, such as posix_memalign() (and not even aligned_alloc() introduced by C11), so nginx uses plain malloc() for supposedly aligned allocations via ngx_memalign(), and too aggressive alignment requirements become important.

The most trivial fix would be to explicitly define the expected alignment for the target platform during compilation, such as ./configure --crossbuild=win32 --with-cc-opt="-DNGX_ALIGNMENT=8" for 32-bit builds (and NGX_ALIGNMENT=16 for 64-bit builds).

It might make sense to hardcode these into src/os/win32/ngx_win32_config.h to simplify things. Alternatively, assuming i386 or amd64 machine in case of --crossbuild=win32 might make sense, so the default alignment will be used.

Not sure if trying to support aligned allocations on win32 make sense. The only interface available seems to be ​_aligned_malloc(). It requires _aligned_free() for deallocation, which is not compatible with free(), so we'll have to use aligned allocations everywhere.

I also noticed that the default alignment when not set is sizeof(long), which is wrong for Win64. It should be sizeof(intptr_t).

The default alignment is the default, and does not pretend to be correct for all possible platforms. On Windows, ​malloc() is expected to provide 8 bytes alignment on 32-bit platforms, and 16 bytes on 64-bit platforms.

On the other hand, given that nginx on Windows does not support platforms with strict alignment requirements, any arbitrary small number will be good enough, including the default sizeof(unsigned long) as used now with ​native compilation.

[Maxim Dounin]

For the record, I've submitted a patch to address this:

​https://mailman.nginx.org/pipermail/nginx-devel/2022-December/QMOITMSRB5XIU5EEYOAIOHW34WEISUJS.html

Review and testing appreciated.

[Maxim Dounin <mdounin@…>]

In 8128:79c04253bc43/nginx:

Win32: i386 now assumed when crossbuilding (ticket #2416).

Previously, NGX_MACHINE was not set when crossbuilding, resulting in
NGX_ALIGNMENT=16 being used in 32-bit builds (if not explicitly set to a
correct value). This in turn might result in memory corruption in
ngx_palloc() (as there are no usable aligned allocator on Windows, and
normal malloc() is used instead, which provides 8 byte alignment on
32-bit platforms).

To fix this, now i386 machine is set when crossbuilding, so nginx won't
assume strict alignment requirements.

[maxim]

Ticket retargeted after milestone closed

[Maxim Dounin]

Fix committed and available in nginx 1.23.4 / 1.24.0, closing this. Thanks to all involved.