Skip to content

Conversation

@maxmouchet
Copy link

@maxmouchet maxmouchet commented Nov 8, 2023

The netinet/udp.h header is included twice in QUIC probe modules: once directly, and once through lib/includes.h.

This causes a build issue on older Linux versions (3.10):

/root/zmapv6/src/probe_modules/module_quic_initial.c: In function _quic_initial_make_packet_:                                                                    
/root/zmapv6/src/probe_modules/module_quic_initial.c:145:12: error: _struct udphdr_ has no member named _uh_sport_                                               
  udp_header->uh_sport =                                                                                                                                         
            ^                                                                                                                                                    
/root/zmapv6/src/probe_modules/module_quic_initial.c:176:12: error: _struct udphdr_ has no member named _uh_ulen_                                                
  udp_header->uh_ulen = ntohs(sizeof(struct udphdr) + payload_len);                                                                                              
            ^                                                                                                                                                    
In file included from /usr/include/bits/byteswap.h:35:0,                                                                                                         
                 from /usr/include/endian.h:60,                                                                                                                  
                 from /usr/include/sys/types.h:216,                                                                                                              
                 from /usr/include/netinet/udp.h:51,                                                                                                             
                 from /root/zmapv6/src/probe_modules/module_quic_initial.c:13:                                                                                   
/root/zmapv6/src/probe_modules/module_quic_initial.c: In function _quic_initial_print_packet_:                                                                   
/root/zmapv6/src/probe_modules/module_quic_initial.c:193:13: error: _struct udphdr_ has no member named _uh_sport_                                               
   ntohs(udph->uh_sport), ntohs(udph->uh_dport),                            

I think this is because in older kernels the udphdr struct was defined through an ifdef:

/* UDP header as specified by RFC 768, August 1980. */
#ifdef __FAVOR_BSD

struct udphdr
{
  u_int16_t uh_sport;                /* source port */
  u_int16_t uh_dport;                /* destination port */
  u_int16_t uh_ulen;                /* udp length */
  u_int16_t uh_sum;                /* udp checksum */
};

#else

struct udphdr
{
  u_int16_t source;
  u_int16_t dest;
  u_int16_t len;
  u_int16_t check;
};
#endif

Whereas it's a union in newer kernels.

Including the header through lib/includes.h fixes the issue because it defines __FAVOR_BSD if not already defined.

Copy link

@ogasser ogasser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me 👍

The main issue is that in these files we were including netinet/udp.h before includes.h. output_modules/module_json.c also includes netinet/udp.h, but does not run into this issue due to includes.h being included first.

So from my side this looks good to be merged. What do you think @zirngibl @sattler ?

@zirngibl
Copy link

Sorry for the late reply but looks good to me as well

@zirngibl zirngibl merged commit b2e8392 into tumi8:master Nov 23, 2023
@maxmouchet maxmouchet deleted the fix-udphdr branch November 23, 2023 13:23
DaveDaCoda pushed a commit to DaveDaCoda/zmap that referenced this pull request Sep 21, 2025
…ap#780)

* fist pass at a debian workflow

* Refactored debian workflow into a common bsd/linux yml. Edited Dockerfile

* attempt tumi8#2

* attempt tumi8#3

* attempt tumi8#4

* attempt tumi8#5

* attempt tumi8#6

* attempt tumi8#7

* attempt tumi8#8

* attempt tumi8#9

* attempt tumi8#10, at least we're getting into the debian dockerfile

* Looks like its working, causing a compile error in get_gateway_linux to see if check fails

* Confirmed, debian workflow is working!

* Removed debugging ls in .yml

* testing arch build

* Added fedora test

* testing Gentoo test

* phillip/773: refactored all the github compile actions to be in a single .yml file

* phillip/773: added arch and gentoo Install instructions

* phillip/773: removed extra space in arch.Dockerfile

* phillip/773: apparently the free-bsd vm is picky about having the run being on the same line
tgesthuizen added a commit to tgesthuizen/zmap that referenced this pull request Nov 7, 2025
GCC is rightfully warning that the output of snprintf might be too
large. Mitigate a possible buffer overflow by providing a bit more
space for string buffers.

┌──── Emitted warning
│ tumi8#9 3.775 /usr/include/x86_64-linux-gnu/bits/stdio2.h:54:10: note: '__builtin___snprintf_chk' output between 9 and 28 bytes into a destination of size 20
│ tumi8#9 3.775    54 |   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
│ tumi8#9 3.775       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
│ tumi8#9 3.775    55 |                                    __glibc_objsize (__s), __fmt,
│ tumi8#9 3.775       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
│ tumi8#9 3.775    56 |                                    __va_arg_pack ());
│ tumi8#9 3.775       |                                    ~~~~~~~~~~~~~~~~~
│ tumi8#9 3.834 /usr/local/src/src/monitor.c: In function 'export_then_update':
│ tumi8#9 3.834 /usr/local/src/src/monitor.c:213:70: warning: '%s' directive output may be truncated writing up to 19 bytes into a region of size 18 [-Wformat-truncation=]
│ tumi8#9 3.834   213 |                 snprintf(exp->time_remaining_str, NUMBER_STR_LEN, " (%s left)",
│ tumi8#9 3.834       |                                                                      ^~
│ tumi8#9 3.834   214 |                          buf);
│ tumi8#9 3.834       |                          ~~~
│ tumi8#9 3.834 In file included from /usr/include/stdio.h:980,
│ tumi8#9 3.834                  from /usr/local/src/src/monitor.c:20:
│ tumi8#9 3.834 In function 'snprintf',
│ tumi8#9 3.834     inlined from 'export_stats' at /usr/local/src/src/monitor.c:213:3,
│ tumi8#9 3.834     inlined from 'export_then_update' at /usr/local/src/src/monitor.c:485:2:
│ tumi8#9 3.834 /usr/include/x86_64-linux-gnu/bits/stdio2.h:54:10: note: '__builtin___snprintf_chk' output between 9 and 28 bytes into a destination of size 20
│ tumi8#9 3.834    54 |   return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
│ tumi8#9 3.834       |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
│ tumi8#9 3.834    55 |                                    __glibc_objsize (__s), __fmt,
│ tumi8#9 3.834       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
│ tumi8#9 3.834    56 |                                    __va_arg_pack ());
│ tumi8#9 3.834       |                                    ~~~~~~~~~~~~~~~~~
└────
tgesthuizen added a commit to tgesthuizen/zmap that referenced this pull request Nov 7, 2025
GCC is rightfully complaining that a variable might be read
uninitialized. Make sure the struct contains deterministic values
instead.

┌──── Emitted warning
│ tumi8#9 4.034 /usr/local/src/src/send.c: In function 'send_run':
│ tumi8#9 4.034 /usr/local/src/src/send.c:415:27: warning: 'current.status' may be used uninitialized [-Wmaybe-uninitialized]
│ tumi8#9 4.034   415 |                 if (!ipv6 && current.status == ZMAP_SHARD_DONE) {
│ tumi8#9 4.034 /usr/local/src/src/send.c:305:18: note: 'current.status' was declared here
│ tumi8#9 4.034   305 |         target_t current;
│ tumi8#9 4.034       |                  ^~~~~~~
│ tumi8#9 4.092 [ 47%] Linking C executable ziterate
│ tumi8#9 4.189 [ 48%] Building C object src/CMakeFiles/zmap.dir/socket.c.o
│ tumi8#9 4.199 [ 49%] Building C object src/CMakeFiles/ztests.dir/socket.c.o
│ tumi8#9 4.278 /usr/local/src/src/send.c: In function 'send_run':
│ tumi8#9 4.278 /usr/local/src/src/send.c:415:27: warning: 'current.status' may be used uninitialized [-Wmaybe-uninitialized]
│ tumi8#9 4.278   415 |                 if (!ipv6 && current.status == ZMAP_SHARD_DONE) {
│ tumi8#9 4.278 /usr/local/src/src/send.c:305:18: note: 'current.status' was declared here
│ tumi8#9 4.278   305 |         target_t current;
│ tumi8#9 4.278       |                  ^~~~~~~
└────
tgesthuizen added a commit to tgesthuizen/zmap that referenced this pull request Nov 7, 2025
send_batch stack allocates buffers as VLAs.
This not only lets GCC complain about it not being able to apply stack
protection for this function, it might also result in stack overflows.
These data structures really belong on the heap.

┌──── Emitted warning
│ tumi8#9 10.32 /usr/local/src/src/send-linux.c: In function 'send_batch':
│ tumi8#9 10.32 /usr/local/src/src/send-linux.c:74:5: warning: stack protector not protecting local variables: variable length buffer [-Wstack-protector]
│ tumi8#9 10.32    74 | int send_batch(sock_t sock, batch_t *batch, int retries)
│ tumi8#9 10.32       |     ^~~~~~~~~~
│ tumi8#9 10.41 [ 97%] Building C object src/CMakeFiles/ztests.dir/socket-linux.c.o
│ tumi8#9 10.42 [ 98%] Building C object src/CMakeFiles/ztests.dir/send-linux.c.o
│ tumi8#9 10.43 [ 98%] Linking C executable zmap
│ tumi8#9 10.45 [ 99%] Building C object src/CMakeFiles/ztests.dir/recv-pcap.c.o
│ tumi8#9 10.67 /usr/local/src/src/send-linux.c: In function 'send_batch':
│ tumi8#9 10.67 /usr/local/src/src/send-linux.c:74:5: warning: stack protector not protecting local variables: variable length buffer [-Wstack-protector]
│ tumi8#9 10.67    74 | int send_batch(sock_t sock, batch_t *batch, int retries)
│ tumi8#9 10.67       |     ^~~~~~~~~~
└────
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.

3 participants