Misc fixes found by Claude Code AI by hauke · Pull Request #42 · openwrt/libubox

hauke · 2026-04-13T23:26:49Z

This PR fixes multiple bugs in the code. I let Claude code do a review of ubus and it found and fixed these.
I manually checked all of them. I think none of them is really critical.

hauke · 2026-04-13T23:28:19Z

 };

-extern bool uloop_cancelled;
+extern volatile sig_atomic_t uloop_cancelled;


This is an ABI change.

Programs accessing uloop_cancelled or uloop_end need recompilation.

I moved this into an own PR. See: #44

I do not want to change the ABI in this PR so I can add this easily to 25.12.

efahl · 2026-04-14T14:17:58Z

 static void setup_strbuf(struct strbuf *s, struct blob_attr *attr, blobmsg_json_format_t cb, void *priv, int indent)
 {
 	s->len = blob_len(attr);
+	if (s->len < 16)


Where did 16 come from? Why not 8 or 32?

Added a documentation.

efahl · 2026-04-14T14:38:18Z

 	if (!data || !len)
 		return -EINVAL;
-	pslen = alloca(policy_len);
+	if (policy_len < 0 || policy_len > 4096)


Another magic number, where did 4096 come from? Is there a #define or declaration somewhere that indicates this is the proper or reasonable value?

That was a suggestion from me.
This alloca could cause an overflow. The policy_len should be pretty short in normal applications even 4k is very big.

efahl · 2026-04-14T14:41:58Z

-		timeout = (ts.tv_sec - cur.tv_sec) * 1000;
-		timeout += (ts.tv_nsec - cur.tv_nsec) / 1000000;
-		if (timeout <= 0)
+		int64_t remaining = ((int64_t)ts.tv_sec - (int64_t)cur.tv_sec) * 1000;


I thought we adhered to the idiom that all declarations must be at the top of the function or scope block.

The local variable holding the realloc() result was declared as struct blob_buf * but buf->buf is void *. Use void * to match the actual type and avoid confusion. Link: openwrt#42 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

The recursive implementation could overflow the stack with deeply chained file lists. Use an iterative loop instead. Link: openwrt#42 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

When tv_nsec equals exactly 1000000000 (1 second), the > comparison misses this case, leaving an invalid timespec value that could cause incorrect timeout calculations. Use >= to properly normalize, matching the correct pattern already used in usock_inet_timeout(). Link: openwrt#42 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

Add error checking for fcntl F_GETFL/F_GETFD calls: if fcntl returns -1 on error, ORing flag bits with -1 produces garbage values. Check the return value before modifying flags. Also remove duplicate #include <poll.h> in usock.c. Link: openwrt#42 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

set_signo() and get_signo() use 1u (unsigned int, 32-bit) as the base for left-shifting by (signo - 1). For real-time signals with signal numbers 33-64, this shift exceeds the width of unsigned int, which is undefined behavior per the C standard. Use UINT64_C(1) to match the uint64_t type of the signums variable and correctly handle all 64 possible signals. Link: openwrt#42 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

…msg_parse() pslen was uint8_t, silently truncating policy name lengths over 255 bytes. This could cause false matches or missed matches when comparing against blobmsg_namelen() which returns uint16_t. Change pslen to uint16_t to match. Also add a bounds check to reject policy_len values above 4096 to guard against stack overflow from the alloca call. Link: openwrt#42 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

usock_timeout_remaining() and poll_restart() compute millisecond differences by multiplying a time_t difference by 1000 and storing the result in int. For large timeouts (>~24 days), this overflows int and produces incorrect values, turning a long timeout into an immediate expiry. Use int64_t for the intermediate calculation and clamp to INT_MAX before returning. Link: openwrt#42 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

Two off-by-one errors cause silent string truncation: 1. The check after the first vsnprintf uses <= instead of <. When the output needs exactly UDEBUG_MIN_ALLOC_LEN chars, vsnprintf returns that count to indicate truncation, but the <= condition skips the reallocation path, silently using the truncated result. 2. After reallocating len+1 bytes, the second vsnprintf is called with len as the size limit. Since vsnprintf writes at most size-1 chars plus a null terminator, the last character is always lost. Pass len+1 to use the full allocated buffer. Link: openwrt#42 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

The strbuf struct used 'int' for its len and pos fields, and blobmsg_puts() took an 'int' len parameter. When formatting very large JSON structures, the check 's->pos + len >= s->len' could overflow, wrapping to a negative value. This caused the bounds check to be bypassed, leading to a heap buffer overflow as data was written past the allocated buffer without triggering a realloc. Fix this by changing the strbuf len/pos fields and the blobmsg_puts() len parameter to size_t, and restructuring the bounds check as 's->len - s->pos <= len' to avoid overflow entirely. Link: openwrt#42 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

setup_strbuf() initialised the strbuf size from blob_len(attr). For attributes with zero or very small payloads this resulted in a malloc(0) call, whose return value is implementation-defined: glibc hands back a non-null pointer, but other allocators may return NULL, which made blobmsg_format_json_with_cb() and blobmsg_format_json_value_with_cb() fail for valid but empty blobs. Floor the initial allocation at 16 bytes so the strbuf is always usable regardless of input size; the buffer still grows on demand in blobmsg_puts(). 16 bytes is enough to hold the literal scalars ("null", "true", "false") and small integer serialisations (int8/int16/int32) without an immediate realloc; int64 and double formatting still triggers one extra realloc, which is acceptable. With s->len now guaranteed to be >= 16 whenever s->buf is non-NULL, the post-format guard if (!s.len) { free(s.buf); return NULL; } in both formatters becomes dead code, so empty/invalid attributes that produced no output are returned as a freshly-malloc'd empty buffer instead of NULL. The original intent of the guard was to detect "nothing was written"; the correct expression for that is !s.pos. Switch the check accordingly so callers continue to see NULL on attributes that produce no output (and the leftover buffer is freed instead of being leaked through the caller as an empty string). Finally, give the floor/grow-slack value a name, STRBUF_MIN_SIZE, and a short comment explaining what it is and why; the same value was already used as the realloc slack in blobmsg_puts(), and using the named constant in both places makes the relationship explicit. Link: openwrt#42 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

blobmsg_alloc_string_buffer() increments maxlen by 1 to account for the null terminator. When maxlen is UINT_MAX, this increment wraps to 0, causing blobmsg_new() to allocate a zero-length payload. A subsequent write to the returned buffer would then overflow the blob's allocated space, corrupting adjacent memory. Add a bounds check before the increment to reject UINT_MAX input. Link: openwrt#42 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

blobmsg_new() sets the BLOB_ATTR_EXTENDED flag in attr->id_len, which is stored in big-endian format. The code used be32_to_cpu() to convert the constant before OR-ing it into the field, but the correct macro is cpu_to_be32() since we are converting from CPU byte order to the on-wire big-endian format. Both macros produce identical results (byte-swapping is its own inverse), so this is a correctness/readability fix with no behavioral change. Link: openwrt#42 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

Using %lf (= %f) with the default precision of 6 fractional decimal digits requires up to 317 characters for negative values close to -DBL_MAX (1 sign + 309 integer digits + 1 decimal point + 6 fraction digits + NUL), but buf[] only holds 317 bytes, leaving no room for the NUL terminator — snprintf truncates the last digit silently. Additionally, 6 fractional digits do not give round-trip accuracy for IEEE 754 doubles; 17 significant digits (%.17g) are required. Switch to %.17g which uses scientific notation for very large or small values, keeping the output well within the buffer, and guarantees round-trip accuracy for all finite doubles. Update the cram fixtures test_blobmsg.t and test_blobmsg_types.t to match the new output. The previous fixtures encoded the buggy behaviour: DBL_MIN serialised as "0.000000" and was silently flattened to zero on the JSON round-trip, while DBL_MAX produced a 309-digit decimal string. Link: openwrt#42 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

Two problems existed in jshn_parse_file(): 1. Integer overflow: sb.st_size is off_t (signed). The expression sb.st_size + 1 triggers signed integer overflow (undefined behaviour) when sb.st_size equals OFF_MAX. Add an explicit bounds check (sb.st_size < 0 || (size_t)sb.st_size >= SIZE_MAX) before the allocation and cast to size_t to make the arithmetic well-defined. 2. Type mismatch in read() comparison: read() returns ssize_t while sb.st_size is off_t. Comparing them directly is implementation- defined when the values differ in sign or width. Store the read() result in a ssize_t variable and compare against sb.st_size explicitly to catch both errors (n < 0) and short reads (n != size). Link: openwrt#42 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

The accumulated allocation size was stored in a signed int, so summing several large size_t arguments could overflow silently and result in a calloc() that is much smaller than intended. Subsequent writes through the per-chunk pointers would then corrupt the heap. Use size_t for the accumulator and explicitly check for overflow on both the per-argument alignment rounding and the running sum. Link: openwrt#42 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

blob_buffer_grow() computed delta and the new buffer size as plain int additions without checking for overflow. With sufficiently large inputs the result could wrap around to a small positive value, so realloc() would shrink the buffer and subsequent writes would overflow the heap. blob_buf_grow() suffered from the same issue: '(buf->buflen + required) > BLOB_ATTR_LEN_MASK' is evaluated after the addition already wraps, so the bound check could be bypassed. Reject negative inputs explicitly and rewrite the bound checks so the arithmetic cannot overflow. Link: openwrt#42 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

blob_pad_len() returns size_t and the value is used as a memory allocation/copy size. Storing it in a signed int could truncate or turn the value negative on platforms or inputs where the padded length exceeds INT_MAX, leading to a too-small malloc() and an out-of-bounds memcpy(). Link: openwrt#42 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

eval_string() copied the pattern into a stack buffer sized via alloca(strlen(pattern) + 1). Patterns come from the JSON script input and may be arbitrarily large; a multi-MB pattern would smash the stack and crash (or, in the worst case, jump past a guard page). Allocate the working copy on the heap instead and free it before returning. While at it, drop the strcpy() in favour of memcpy() with the length we just computed. Link: openwrt#42 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

'maxlen + 1' wraps to 0 when maxlen is UINT_MAX, and the mixed signed/unsigned subtraction that produced 'required' could store a huge unsigned value into a signed int. In either case the function might return without growing the buffer while the caller assumes maxlen bytes are now available, leading to an out-of-bounds write. Reject UINT_MAX explicitly (matching blobmsg_alloc_string_buffer()) and rewrite the size comparison so the arithmetic stays in unsigned space and is bounded. Link: openwrt#42 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

vsnprintf() returns the length the formatted output would have had if the buffer were unlimited. When that length is INT_MAX, the following 'maxlen + 1' wraps to a negative value (-> huge size_t), so malloc() either fails or, on 32-bit systems, returns a far too small allocation that the subsequent vsnprintf() then overruns. Reject INT_MAX explicitly in both call sites that allocate the oversized buffer. Link: openwrt#42 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

fread() returning 0 was treated as end of file, but it can also mean a read error. In that case the loop terminated and md5_end() ran on the partial data, so the caller received a md5 sum that did not correspond to the file content and no error was reported. Distinguish EOF from read errors with ferror() and bail out with -1 on a real failure. Also use size_t for the return value of fread(). Link: openwrt#42 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

AndyChiang888 · 2026-05-17T16:28:47Z

9b48801 causes uhttpd crash randomly. @hauke

mkowalski · 2026-05-18T09:27:35Z

"Hey Claude, fix this codebase. Make no mistakes" 🙃

hauke commented Apr 13, 2026

View reviewed changes

efahl reviewed Apr 14, 2026

View reviewed changes

hauke force-pushed the fix-ai-bugs branch 2 times, most recently from fa970ce to a076257 Compare April 26, 2026 22:25

hauke marked this pull request as ready for review April 26, 2026 22:45

hauke and others added 21 commits May 3, 2026 22:16

hauke force-pushed the fix-ai-bugs branch from a076257 to 1501e60 Compare May 3, 2026 20:18

openwrt-bot merged commit 1501e60 into openwrt:master May 3, 2026
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc fixes found by Claude Code AI#42

Misc fixes found by Claude Code AI#42
openwrt-bot merged 21 commits into
openwrt:masterfrom
hauke:fix-ai-bugs

hauke commented Apr 13, 2026

Uh oh!

hauke Apr 13, 2026

Uh oh!

hauke Apr 23, 2026

Uh oh!

efahl Apr 14, 2026

Uh oh!

hauke Apr 23, 2026

Uh oh!

efahl Apr 14, 2026

Uh oh!

hauke Apr 16, 2026

Uh oh!

efahl Apr 14, 2026

Uh oh!

hauke Apr 23, 2026

Uh oh!

Uh oh!

AndyChiang888 commented May 17, 2026

Uh oh!

mkowalski commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

hauke commented Apr 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndyChiang888 commented May 17, 2026

Uh oh!

mkowalski commented May 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants