Skip to content

Minor fixes#60

Merged
gasbytes merged 5 commits intowolfSSL:masterfrom
danielinux:minor-fixes
Mar 3, 2026
Merged

Minor fixes#60
gasbytes merged 5 commits intowolfSSL:masterfrom
danielinux:minor-fixes

Conversation

@danielinux
Copy link
Member

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

Also addresses Fenrir/242
Check FIFO is actually empty before clearing CB_EVENT_READABLE.

Fix for F/243
Copilot AI review requested due to automatic review settings March 3, 2026 10:06
@danielinux danielinux requested a review from gasbytes March 3, 2026 10:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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-noeth target.

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, reach tok_len == MAX_DNS_NAME_LEN, and then tok_len++ will make the encoded name length 256 bytes, exceeding the 255-byte DNS maximum and shifting q past the intended bounds. Update the bounds checks to account for the final terminator (e.g., ensure tok_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_str is zeroed twice (once before the block and again inside it), and the payload length calculation still uses the magic constant 8 instead of UDP_HEADER_LEN. Removing the redundant memset and using UDP_HEADER_LEN would 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
Copy link
Contributor

gasbytes commented Mar 3, 2026

lgtm, verified and reviewed locally with gh cli.

@gasbytes gasbytes merged commit 46c46d8 into wolfSSL:master Mar 3, 2026
19 checks passed
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