Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert 0-sized arrays to true flexible array members #1240

Merged
merged 2 commits into from
May 7, 2024
Merged

Conversation

ac000
Copy link
Member

@ac000 ac000 commented May 1, 2024

Declaring a 0-sized array (e.g 'char arr[0];') as the last member of a
structure is a GNU extension that was used to implement flexible array
members (FAMs) before they were standardised in C99 as simply '[]'.

The GNU extension itself was introduced to work around a hack of
declaring 1-sized arrays to mean a variable-length object. The advantage
of the 0-sized (and true FAMs) is that they don't count towards the size
of the structure.

Unit already declares some true FAMs, but it also declared some 0-sized
arrays.

Converting these 0-sized arrays to true FAMs is not only good for
consistency but will also allow better compiler checks now (as in a C99
FAM must be the last member of a structure and the compiler will warn
otherwise) and in the future as doing this fixes a bunch of warnings
(treated as errors in Unit by default) when compiled with

  -O2 -Warray-bounds -Wstrict-flex-arrays -fstrict-flex-arrays=3

(Note -Warray-bounds is enabled by -Wall and -Wstrict-flex-arrays seems
to also be enabled via -Wall -Wextra, the -02 is required to make
-fstrict-flex-arrays more effective, =3 is the default on at least GCC
14)

such as

  CC     build/src/nxt_upstream.o
src/nxt_upstream.c: In function ‘nxt_upstreams_create’:
src/nxt_upstream.c:56:18: error: array subscript i is outside array bounds of ‘nxt_upstream_t[0]’ {aka ‘struct nxt_upstream_s[]’} [-Werror=array-bounds=]
   56 |         string = nxt_str_dup(mp, &upstreams->upstream[i].name, &name);
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/nxt_upstream.c:9:
src/nxt_upstream.h:55:48: note: while referencing ‘upstream’
   55 |     nxt_upstream_t                             upstream[0];
      |                                                ^~~~~~~~

Making our flexible array members proper C99 FAMs and ensuring any >0
sized trailing arrays in structures are really normal arrays will allow
to enable various compiler options (such as the above and more) that
will help keep our array usage safe.

Changing 0-sized arrays to FAMs should have no effect on structure
layouts/sizes (they both have a size of 0, although doing a sizeof() on
a FAM will result in a compiler error).

Looking at pahole(1) output for the nxt_http_route_ruleset_t structure
for the [0] and [] cases...

$ pahole -C nxt_http_route_ruleset_t /tmp/build/src/nxt_http_route.o
typedef struct {
        uint32_t           items;                /*     0     4 */

        /* XXX 4 bytes hole, try to pack */

        nxt_http_route_rule_t * rule[];          /*     8     0 */

        /* size: 8, cachelines: 1, members: 2 */
        /* sum members: 4, holes: 1, sum holes: 4 */
        /* last cacheline: 8 bytes */
} nxt_http_route_ruleset_t;
$ pahole -C nxt_http_route_ruleset_t build/src/nxt_http_route.o
typedef struct {
        uint32_t           items;                /*     0     4 */

        /* XXX 4 bytes hole, try to pack */

        nxt_http_route_rule_t * rule[];          /*     8     0 */

        /* size: 8, cachelines: 1, members: 2 */
        /* sum members: 4, holes: 1, sum holes: 4 */
        /* last cacheline: 8 bytes */
} nxt_http_route_ruleset_t;

Also checking with the size(1) command on the effected object files
shows no changes to their sizes

$ for file in build/src/nxt_upstream.o \
            build/src/nxt_upstream_round_robin.o \
            build/src/nxt_h1proto.o \
            build/src/nxt_http_route.o \
            build/src/nxt_http_proxy.o \
            build/src/python/*.o; do \
            size -G /tmp/${file} $file; echo; done
      text       data        bss      total filename
       640        418          0       1058 /tmp/build/src/nxt_upstream.o
       640        418          0       1058 build/src/nxt_upstream.o

      text       data        bss      total filename
       929        351          0       1280 /tmp/build/src/nxt_upstream_round_robin.o
       929        351          0       1280 build/src/nxt_upstream_round_robin.o

      text       data        bss      total filename
     11707       8281         16      20004 /tmp/build/src/nxt_h1proto.o
     11707       8281         16      20004 build/src/nxt_h1proto.o

      text       data        bss      total filename
      8319       3101          0      11420 /tmp/build/src/nxt_http_route.o
      8319       3101          0      11420 build/src/nxt_http_route.o

      text       data        bss      total filename
      1495       1056          0       2551 /tmp/build/src/nxt_http_proxy.o
      1495       1056          0       2551 build/src/nxt_http_proxy.o

      text       data        bss      total filename
      4321       2895          0       7216 /tmp/build/src/python/nxt_python_asgi_http-python.o
      4321       2895          0       7216 build/src/python/nxt_python_asgi_http-python.o

      text       data        bss      total filename
      4231       2266          0       6497 /tmp/build/src/python/nxt_python_asgi_lifespan-python.o
      4231       2266          0       6497 build/src/python/nxt_python_asgi_lifespan-python.o

      text       data        bss      total filename
     12051       6090          8      18149 /tmp/build/src/python/nxt_python_asgi-python.o
     12051       6090          8      18149 build/src/python/nxt_python_asgi-python.o

      text       data        bss      total filename
        28       1963        432       2423 /tmp/build/src/python/nxt_python_asgi_str-python.o
        28       1963        432       2423 build/src/python/nxt_python_asgi_str-python.o

      text       data        bss      total filename
      5818       3518          0       9336 /tmp/build/src/python/nxt_python_asgi_websocket-python.o
      5818       3518          0       9336 build/src/python/nxt_python_asgi_websocket-python.o
      text       data        bss      total filename
      4391       2089        168       6648 /tmp/build/src/python/nxt_python-python.o
      4391       2089        168       6648 build/src/python/nxt_python-python.o

      text       data        bss      total filename
      9095       5909        152      15156 /tmp/build/src/python/nxt_python_wsgi-python.o
      9095       5909        152      15156 build/src/python/nxt_python_wsgi-python.o

@ac000 ac000 force-pushed the fla branch 2 times, most recently from 7e67555 to f84b5ba Compare May 1, 2024 19:08
@ac000 ac000 marked this pull request as ready for review May 1, 2024 19:42
@ac000 ac000 requested a review from hongzhidao May 1, 2024 19:43
ac000 added 2 commits May 7, 2024 02:43
Since commit 0b5223e ("Disable strict-aliasing in clang by default")
we explicitly always build with -fno-strict-aliasing so there's no need
to set it independently in auto/modules/wasm

Signed-off-by: Andrew Clayton <[email protected]>
Declaring a 0-sized array (e.g 'char arr[0];') as the last member of a
structure is a GNU extension that was used to implement flexible array
members (FAMs) before they were standardised in C99 as simply '[]'.

The GNU extension itself was introduced to work around a hack of
declaring 1-sized arrays to mean a variable-length object. The advantage
of the 0-sized (and true FAMs) is that they don't count towards the size
of the structure.

Unit already declares some true FAMs, but it also declared some 0-sized
arrays.

Converting these 0-sized arrays to true FAMs is not only good for
consistency but will also allow better compiler checks now (as in a C99
FAM *must* be the last member of a structure and the compiler will warn
otherwise) and in the future as doing this fixes a bunch of warnings
(treated as errors in Unit by default) when compiled with

  -O2 -Warray-bounds -Wstrict-flex-arrays -fstrict-flex-arrays=3

(Note -Warray-bounds is enabled by -Wall and -Wstrict-flex-arrays seems
to also be enabled via -Wall -Wextra, the -02 is required to make
-fstrict-flex-arrays more effective, =3 is the default on at least GCC
14)

such as

  CC     build/src/nxt_upstream.o
src/nxt_upstream.c: In function ‘nxt_upstreams_create’:
src/nxt_upstream.c:56:18: error: array subscript i is outside array bounds of ‘nxt_upstream_t[0]’ {aka ‘struct nxt_upstream_s[]’} [-Werror=array-bounds=]
   56 |         string = nxt_str_dup(mp, &upstreams->upstream[i].name, &name);
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/nxt_upstream.c:9:
src/nxt_upstream.h:55:48: note: while referencing ‘upstream’
   55 |     nxt_upstream_t                             upstream[0];
      |                                                ^~~~~~~~

Making our flexible array members proper C99 FAMs and ensuring any >0
sized trailing arrays in structures are really normal arrays will allow
to enable various compiler options (such as the above and more) that
will help keep our array usage safe.

Changing 0-sized arrays to FAMs should have no effect on structure
layouts/sizes (they both have a size of 0, although doing a sizeof() on
a FAM will result in a compiler error).

Looking at pahole(1) output for the nxt_http_route_ruleset_t structure
for the [0] and [] cases...

$ pahole -C nxt_http_route_ruleset_t /tmp/build/src/nxt_http_route.o
typedef struct {
        uint32_t           items;                /*     0     4 */

        /* XXX 4 bytes hole, try to pack */

        nxt_http_route_rule_t * rule[];          /*     8     0 */

        /* size: 8, cachelines: 1, members: 2 */
        /* sum members: 4, holes: 1, sum holes: 4 */
        /* last cacheline: 8 bytes */
} nxt_http_route_ruleset_t;
$ pahole -C nxt_http_route_ruleset_t build/src/nxt_http_route.o
typedef struct {
        uint32_t           items;                /*     0     4 */

        /* XXX 4 bytes hole, try to pack */

        nxt_http_route_rule_t * rule[];          /*     8     0 */

        /* size: 8, cachelines: 1, members: 2 */
        /* sum members: 4, holes: 1, sum holes: 4 */
        /* last cacheline: 8 bytes */
} nxt_http_route_ruleset_t;

Also checking with the size(1) command on the effected object files
shows no changes to their sizes

$ for file in build/src/nxt_upstream.o \
	build/src/nxt_upstream_round_robin.o \
	build/src/nxt_h1proto.o \
	build/src/nxt_http_route.o \
	build/src/nxt_http_proxy.o \
	build/src/python/*.o; do \
	size -G /tmp/${file} $file; echo; done
      text       data        bss      total filename
       640        418          0       1058 /tmp/build/src/nxt_upstream.o
       640        418          0       1058 build/src/nxt_upstream.o

      text       data        bss      total filename
       929        351          0       1280 /tmp/build/src/nxt_upstream_round_robin.o
       929        351          0       1280 build/src/nxt_upstream_round_robin.o

      text       data        bss      total filename
     11707       8281         16      20004 /tmp/build/src/nxt_h1proto.o
     11707       8281         16      20004 build/src/nxt_h1proto.o

      text       data        bss      total filename
      8319       3101          0      11420 /tmp/build/src/nxt_http_route.o
      8319       3101          0      11420 build/src/nxt_http_route.o

      text       data        bss      total filename
      1495       1056          0       2551 /tmp/build/src/nxt_http_proxy.o
      1495       1056          0       2551 build/src/nxt_http_proxy.o

      text       data        bss      total filename
      4321       2895          0       7216 /tmp/build/src/python/nxt_python_asgi_http-python.o
      4321       2895          0       7216 build/src/python/nxt_python_asgi_http-python.o

      text       data        bss      total filename
      4231       2266          0       6497 /tmp/build/src/python/nxt_python_asgi_lifespan-python.o
      4231       2266          0       6497 build/src/python/nxt_python_asgi_lifespan-python.o

      text       data        bss      total filename
     12051       6090          8      18149 /tmp/build/src/python/nxt_python_asgi-python.o
     12051       6090          8      18149 build/src/python/nxt_python_asgi-python.o

      text       data        bss      total filename
        28       1963        432       2423 /tmp/build/src/python/nxt_python_asgi_str-python.o
        28       1963        432       2423 build/src/python/nxt_python_asgi_str-python.o

      text       data        bss      total filename
      5818       3518          0       9336 /tmp/build/src/python/nxt_python_asgi_websocket-python.o
      5818       3518          0       9336 build/src/python/nxt_python_asgi_websocket-python.o

      text       data        bss      total filename
      4391       2089        168       6648 /tmp/build/src/python/nxt_python-python.o
      4391       2089        168       6648 build/src/python/nxt_python-python.o

      text       data        bss      total filename
      9095       5909        152      15156 /tmp/build/src/python/nxt_python_wsgi-python.o
      9095       5909        152      15156 build/src/python/nxt_python_wsgi-python.o

Link: <https://lwn.net/Articles/908817/>
Link: <https://people.kernel.org/kees/bounded-flexible-arrays-in-c>
Signed-off-by: Andrew Clayton <[email protected]>
@ac000
Copy link
Member Author

ac000 commented May 7, 2024

Rebased with master

@ac000 ac000 merged commit e2a09c7 into nginx:master May 7, 2024
19 checks passed
@ac000 ac000 deleted the fla branch May 7, 2024 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants