Merged
Conversation
Also addresses Fenrir/242
Check FIFO is actually empty before clearing CB_EVENT_READABLE. Fix for F/243
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a set of static-analysis findings across wolfIP, primarily targeting safer length handling, non-Ethernet build compatibility, socket API argument validation, and more robust DNS query name serialization.
Changes:
- Harden UDP debug printing against short/invalid UDP lengths and add a regression unit test.
- Fix ICMP readable-event clearing logic to avoid clearing when the RX FIFO still has queued packets (with unit test).
- Improve non-Ethernet build compatibility by removing/guarding Ethernet-only assumptions and adding a compilation-only
unit-noethtarget.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/wolfip_debug.c | Adds a length guard to prevent UDP payload underflow/OOB reads when printing debug info. |
| src/wolfip.c | Fixes Ethernet header sizing assumptions, ICMP readable event handling, getpeername arg validation, Ethernet guards in ICMP reply path, and strengthens DNS label/name length checks. |
| src/test/unit/unit_noeth.c | Adds a compilation-only harness to ensure the stack builds with ETHERNET disabled. |
| src/test/unit/unit.c | Adds unit tests for the new error paths/edge cases; also toggles UDP debug compilation flags. |
| Makefile | Adds a unit-noeth target to compile the non-Ethernet harness. |
Comments suppressed due to low confidence (2)
src/wolfip.c:5744
- The DNS QNAME length checks don’t reserve space for the terminating 0 byte. As written, a name can pass
tok_len + label_len + 1 > MAX_DNS_NAME_LEN, reachtok_len == MAX_DNS_NAME_LEN, and thentok_len++will make the encoded name length 256 bytes, exceeding the 255-byte DNS maximum and shiftingqpast the intended bounds. Update the bounds checks to account for the final terminator (e.g., ensuretok_len + label_len + 2 <= MAX_DNS_NAME_LEN, or add a check before incrementing/writing the terminator).
if (strlen(dname) > MAX_DNS_NAME_LEN) return -22; /* Invalid arguments */
if (s->dns_server == 0) return -101; /* Network unreachable: No DNS server configured */
if (s->dns_id != 0) return -16; /* DNS query already in progress */
if (s->dns_udp_sd <= 0) {
s->dns_udp_sd = wolfIP_sock_socket(s, AF_INET, IPSTACK_SOCK_DGRAM, WI_IPPROTO_UDP);
if (s->dns_udp_sd < 0)
return -1;
wolfIP_register_callback(s, s->dns_udp_sd, dns_callback, s);
}
s->dns_id = wolfIP_getrandom();
*id = s->dns_id;
memset(buf, 0, 512);
s->dns_query_type = (qtype == DNS_PTR) ? DNS_QUERY_TYPE_PTR : DNS_QUERY_TYPE_A;
hdr = (struct dns_header *)buf;
hdr->id = ee16(s->dns_id);
hdr->flags = ee16(DNS_QUERY);
hdr->qdcount = ee16(1);
hdr->flags = ee16(DNS_RD);
/* Prepare the DNS query name */
q_name = (char *)(buf + sizeof(struct dns_header));
tok_start = (char *)dname;
while(*tok_start) {
tok_end = tok_start;
while ((*tok_end != '.') && (*tok_end != 0)) {
tok_end++;
}
label_len = (uint32_t)(tok_end - tok_start);
if (label_len > MAX_DNS_LABEL_LEN) return -22;
if (tok_len + label_len + 1 > MAX_DNS_NAME_LEN) return -22;
*q_name = (char)label_len;
q_name++;
memcpy(q_name, tok_start, label_len);
q_name += label_len;
tok_len += label_len + 1;
if (*tok_end == 0)
break;
tok_start = tok_end + 1;
}
*q_name = 0;
tok_len++;
q = (struct dns_question *)(buf + sizeof(struct dns_header) + tok_len);
q->qtype = ee16(qtype);
src/wolfip_debug.c:98
- Within
wolfIP_print_udp,payload_stris zeroed twice (once before the block and again inside it), and the payload length calculation still uses the magic constant8instead ofUDP_HEADER_LEN. Removing the redundantmemsetand usingUDP_HEADER_LENwould keep the debug helper simpler and less error-prone.
memset(payload_str, '\0', sizeof(payload_str));
{
/* show first 16 printable chars of payload */
uint16_t max_len = 16;
size_t i = 0;
size_t print_len = 0;
if (len <= UDP_HEADER_LEN)
return;
print_len = (len - 8) < max_len ? (len - 8): max_len;
memset(payload_str, '\0', sizeof(payload_str));
memcpy(payload_str, udp->data, print_len);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gasbytes
approved these changes
Mar 3, 2026
Contributor
|
lgtm, verified and reviewed locally with gh cli. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Address several improvements suggested by the static analysis tools.
F-242 - Fixes for non-ethernet builds
F-243 - ICMP: don't clear readable flag if fifo not empty
F-74 - fix potential buffer overflow in wolfIP_print_udp
F-138 - fix arguments check in wolfIP_sock_getpeer
F-140 - fix token serializations in dns_send_query