net: http: server: Avoid compiler warnings for zero-length-arrays

[clamattia]

Avoid compiler warnings for zero-length-arrays in the http-server.

Note: warnings only visible if CONFIG_NO_OPTIMIZATIONS is selected.

Note2: Alternative approach would be to set length to 1 instead of 0. Would that be prefered?

[jukkar]

I just wonder why the compiler thinks there is an error here, what is the actual issue?

[mrodgers-witekio]

This was originally implemented with the buffer sizes set to zero if the feature is disabled so that the code referencing the headers and buffer fields does not need to be conditionally compiled, and we can just enable or disable it using if(IS_ENABLED(CONFIG_HTTP_SERVER_HEADER_CAPTURE)) instead. As well as being more readable this has the advantage that the code is always compile-checked regardless of whether the option is enabled, and just gets optimised out at compile/link time if the option is disabled.

Based on this, I think we're likely to run into some build failures if we just #ifdef out the struct fields in the header without touching the C files (CI hasn't run yet so this should show up any issues we do have).

[clamattia]

This was originally implemented with the buffer sizes set to zero if the feature is disabled so that the code referencing the headers and buffer fields does not need to be conditionally compiled, and we can just enable or disable it using if(IS_ENABLED(CONFIG_HTTP_SERVER_HEADER_CAPTURE)) instead. As well as being more readable this has the advantage that the code is always compile-checked regardless of whether the option is enabled, and just gets optimised out at compile/link time if the option is disabled.

Based on this, I think we're likely to run into some build failures if we just #ifdef out the struct fields in the header without touching the C files (CI hasn't run yet so this should show up any issues we do have).

Make sense. Should we put there size to 1 instead of 0?

Edit: AFAIK in C every object has size at least = 1 anyway to ensure unique addresses?

@jukkar

zephyr/subsys/net/lib/http/http_server_http1.c: In function ‘check_user_request_headers’:
##[error]zephyr/subsys/net/lib/http/http_server_http1.c:681:25: error: ‘strcpy’ writing 1 or more bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=]
  681 |                         strcpy(dest, header->name);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from zephyr/include/zephyr/net/http/service.h:22,
                 from zephyr/subsys/net/lib/http/http_server_http1.c:21:
zephyr/include/zephyr/net/http/server.h:379:23: note: destination object ‘buffer’ of size 0
  379 |         unsigned char buffer[HTTP_SERVER_CAPTURE_HEADER_BUFFER_SIZE];
      |                       ^~~~~~
zephyr/subsys/net/lib/http/http_server_http1.c: In function ‘populate_user_request_header’:
##[error]zephyr/subsys/net/lib/http/http_server_http1.c:759:9: error: ‘strcpy’ writing 1 or more bytes into a region of size 0 overflows the destination [-Werror=stringop-overflow=]
  759 |         strcpy(dest, buf);
      |         ^~~~~~~~~~~~~~~~~
zephyr/include/zephyr/net/http/server.h:379:23: note: destination object ‘buffer’ of size 0
  379 |         unsigned char buffer[HTTP_SERVER_CAPTURE_HEADER_BUFFER_SIZE];
      |                       ^~~~~~

[jukkar]

We could try to set the value to [] which is more portable than [0] or [1]

[jukkar]

This is related to #87650

[mrodgers-witekio]

OK, it looks like the warning is a false positive since the buffer length is checked before the call to strcpy in both cases. Do you have any other configurations set, I'm not able to reproduce the warning when building for native_sim with CONFIG_NO_OPTIMIZATIONS set?

I wonder if swapping the strcpy calls with a memcpy(dest, buf, value_len + 1) would eliminate the warning? Might be marginally quicker too since we already know the length of the string.

[clamattia]

OK, it looks like the warning is a false positive since the buffer length is checked before the call to strcpy in both cases. Do you have any other configurations set, I'm not able to reproduce the warning when building for native_sim with CONFIG_NO_OPTIMIZATIONS set?

I wonder if swapping the strcpy calls with a memcpy(dest, buf, value_len + 1) would eliminate the warning? Might be marginally quicker too since we already know the length of the string.

Thanks for the quick response. I will double check.

[clamattia]

OK, it looks like the warning is a false positive since the buffer length is checked before the call to strcpy in both cases. Do you have any other configurations set, I'm not able to reproduce the warning when building for native_sim with CONFIG_NO_OPTIMIZATIONS set?

I wonder if swapping the strcpy calls with a memcpy(dest, buf, value_len + 1) would eliminate the warning? Might be marginally quicker too since we already know the length of the string.

CONFIG_NET_SHELL_HTTP_SERVER_SUPPORTED=y
CONFIG_HTTP_PARSER=y
CONFIG_HTTP_PARSER_URL=y
# CONFIG_HTTP_PARSER_STRICT is not set
# CONFIG_HTTP_CLIENT is not set
CONFIG_HTTP_SERVER=y
CONFIG_HTTP_SERVER_STACK_SIZE=8192
CONFIG_HTTP_SERVER_NUM_SERVICES=1
CONFIG_HTTP_SERVER_MAX_CLIENTS=3
CONFIG_HTTP_SERVER_MAX_STREAMS=10
CONFIG_HTTP_SERVER_CLIENT_BUFFER_SIZE=512
CONFIG_HTTP_SERVER_HUFFMAN_DECODE_BUFFER_SIZE=256
CONFIG_HTTP_SERVER_MAX_URL_LENGTH=256
CONFIG_HTTP_SERVER_MAX_CONTENT_TYPE_LENGTH=64
CONFIG_HTTP_SERVER_MAX_HEADER_LEN=32
CONFIG_HTTP_SERVER_HTTP2_MAX_HEADER_FRAME_LEN=64
# CONFIG_HTTP_SERVER_CAPTURE_HEADERS is not set
CONFIG_HTTP_SERVER_CLIENT_INACTIVITY_TIMEOUT=10
# CONFIG_HTTP_SERVER_WEBSOCKET is not set
CONFIG_HTTP_SERVER_RESOURCE_WILDCARD=y
CONFIG_HTTP_SERVER_RESTART_DELAY=1000
# CONFIG_HTTP_SERVER_REPORT_FAILURE_REASON is not set
CONFIG_HTTP_SERVER_COMPRESSION=y
CONFIG_HTTP=y
CONFIG_NET_HTTP_LOG_LEVEL=0
CONFIG_NET_HTTP_SERVER_LOG_LEVEL=0

[clamattia]

We could try to set the value to [] which is more portable than [0] or [1]

Does not work in this case:

zephyr/include/zephyr/net/http/server.h:379:23: error: flexible array member not at end of struct
  379 |         unsigned char buffer[HTTP_SERVER_CAPTURE_HEADER_BUFFER_SIZE];
      |                       ^~~~~~
zephyr/include/zephyr/net/http/server.h:382:28: error: flexible array member not at end of struct
  382 |         struct http_header headers[HTTP_SERVER_CAPTURE_HEADER_COUNT];
      |                            ^~~~~~~

[clamattia]

Adjusted to use memcpy. It is a bit a funny fix, since we would still overwrite in case the buffer is 0 (not the case when the function gets actually used). Somehow it makes the compiler believe we know what we are doing :D

[jukkar]

The FLEXIBLE_ARRAY_DECLARE macro has some trickery to overcome the last element in the struct issue

zephyr/include/zephyr/sys/util.h

Line 142 in d02bc50

#define FLEXIBLE_ARRAY_DECLARE(type, name) \

I am not suggesting that we use that macro here, just a note that it can be done.
Perhaps we should silence the compiler in this case like what is done in #87489

[mrodgers-witekio]

If the compiler is happy with memcpy instead, then that's good with me