Skip to content

Assorted bug fixes - #464

Merged
jow- merged 16 commits into
masterfrom
fix/blogic-fixes-1
Oct 3, 2026
Merged

Assorted bug fixes#464
jow- merged 16 commits into
masterfrom
fix/blogic-fixes-1

Conversation

@jow-

@jow- jow- commented Oct 3, 2026

Copy link
Copy Markdown
Collaborator

A batch of bug fixes across the VM, compiler and stdlib, each with a short description of the misbehaviour and, where measured, how it was verified.

vm

  • give division by zero the right sign, and NaN for zero over zero — -1 / 0 yielded Infinity instead of -Infinity, and 0 / 0 yielded Infinity instead of NaN.

compiler

  • do not overflow int building the wildcard import operand — 0xffff << 16 is a plain int constant, so the shift overflows.
  • unwind the nested function compiler on a bad argument list — avoid continuing compilation with a corrupt state.

struct

  • accumulate signed integers in an unsigned type — avoid signed overflow while accumulating.
  • decode any non-zero byte as true in native mode — only 1 was treated as true.

fs

  • report a write that only fails when the file is closed — fwrite() buffers, so ENOSPC/EIO/EDQUOT surfacing in fclose() were discarded and the call reported success.
  • keep the bytes a short read already delivered — a short read dropped the bytes already consumed.

uloop

  • redirect the task child's standard descriptors to /dev/null — dup2() arguments were swapped, so /dev/null was never installed and task children inherited the parent's stdio (corrupting files opened first by daemons that log to stderr).
  • close the task output pipe even without an input callback — the cleanup sat inside an input_fd >= 0 check, leaking the read end for the common callback shapes.
  • release the handle when uloop_fd_add() fails — the error path dropped only one of two references and left the persistent flag set, so the resource was uncollectable.
  • read task messages without losing them to EAGAIN — the pipe was registered non-blocking but the reader assumed blocking; partial messages were discarded and the stream left misaligned (11 of 25 runs lost a 100 KB message; 25 of 25 after the fix).

resolv

  • file each NS, CNAME and PTR record under its own type — records were filed under the wrong type.
  • release the TXT buffer when the record is malformed — leak on the error path.

zlib

  • inflate every member of a concatenated stream — only the first member of a multi-member stream was inflated.

io

  • read the current termios before applying attributes — tcsetattr() merged the attrs object into an uninitialised stack struct, so omitted fields carried garbage to the kernel (measured: setting lflag alone cleared CBAUD to B0 and hung up a real serial line).

socket

  • fix bind() ignoring port argument — the optional port parameter was discarded, so s.bind('0.0.0.0', 44444) got an ephemeral port.

blogic added 16 commits October 3, 2026 15:24
@blogic @jow-
ucode: fix socket bind() ignoring port argument
068e7b2 
uc_socket_inst_bind() discarded the optional port parameter, so
s.bind('0.0.0.0', 44444) ended up with an ephemeral kernel-assigned
port. Mirror the port handling already done in connect().

Signed-off-by: John Crispin <john@phrozen.org>
(applied from the patch file added by blogic/staging commit 90f0f68)
@blogic @jow-
io: read the current termios before applying attributes
a4da50d 
uc_io_tcsetattr() declares a struct termios on the stack and merges in
only the fields present in the attrs object, so every field the caller
omits is whatever the stack happened to hold, and that is what reaches the
kernel.

  import * as io from 'io';

  let h = io.open('/dev/ttyUSB0', io.O_RDWR | io.O_NOCTTY);
  h.tcsetattr({ lflag: 0 });

Measured: setting lflag alone moved cflag from 191 to 176, clearing the
CBAUD field to B0, which drops DTR and hangs up a real serial line. The
canonical get/modify/set pattern is affected too, since c_line is not
among the fields tcgetattr() reports.

Read the current attributes first, which is what the comment above the
block already claims and what lib/serial.c's uc_serial_setattr() does.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
uloop: redirect the task child's standard descriptors to /dev/null
38f21be 
patch_devnull() opens /dev/null and then calls dup2(fd, devnull), which
copies the standard descriptor onto the new one, the opposite of what is
meant, and close(fd) then shuts the descriptor it was supposed to
redirect. /dev/null is never installed anywhere.

The three calls cascade, because each open() is handed the descriptor the
previous close() released. In every uloop.task child stdin becomes the
parent's stdout, vm->output is fdopen()ed on what is now the parent's
stderr so print() writes there, there is no stderr at all, and the child's
first open() is given fd 2, so everything it later writes to stderr goes
into that file.

  import * as uloop from 'uloop';
  import * as fs from 'fs';

  uloop.init();
  uloop.task(function(pipe) {
      let f = fs.open('/tmp/state.json', 'w');
      warn("diagnostic: retrying once\n");
      f.write('{"state":"ok"}\n');
      f.close();
      return 1;
  }, function(msg) {});
  uloop.timer(3000, () => uloop.end());
  uloop.run();

leaves /tmp/state.json starting with the diagnostic line rather than the
JSON. Any daemon whose logging writes to stderr as well as syslog corrupts
whatever file a task opens first.

Swap the arguments and close the temporary descriptor instead.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
uloop: close the task output pipe even without an input callback
af8a383 
uc_uloop_task_clear() closes the output pipe and removes the process
watcher inside `if (task->input_fd >= 0)`, but uc_uloop_task() sets
input_fd to -1 whenever no input callback was given. The two forms that
omit one therefore never reach uloop_fd_close(&task->output), and leak the
read end of the output pipe.

  import * as uloop from 'uloop';

  uloop.init();
  for (let i = 0; i < 40; i++)
      uloop.task(function(pipe) { return 1; }, function(msg) {});
  uloop.timer(2000, () => uloop.end());
  uloop.run();

leaks 40 descriptors. Collecting a result through an output callback is
the documented pattern and an input callback is the rarity, so the leaking
shapes are the usual ones. Against a 1024 descriptor limit a daemon
running one task per event stops being able to fork, connect or open a
file after about a thousand of them, and uloop.task() then returns null
for good.

Close the pipe and drop the process watcher regardless; both calls already
guard against being invoked twice.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
uloop: release the handle when uloop_fd_add() fails
e6e0300 
uc_uloop_alloc() hands back a resource carrying two references, the one
the caller receives and the self-reference the callback machinery holds,
and marks it persistent so the collector treats it as a root. The
uloop_fd_add() error path drops only one of the two and leaves the
persistent flag set, so the resource is unreachable, uncollectable and
never freed.

  import * as uloop from 'uloop';
  import * as fs from 'fs';

  uloop.init();
  let f = fs.open('/etc/hostname', 'r');
  for (let i = 0; i < 1000; i++)
      uloop.handle(f, function(e) {}, uloop.ULOOP_READ);

leaks about 165 bytes each time. epoll refuses a regular file with EPERM
and an already-registered descriptor with EEXIST, so a daemon that watches
whatever it is handed reaches this on ordinary input.

Tear the resource down the way uc_uloop_cb_free() does and release the
creation reference as well.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
uloop: read task messages without losing them to EAGAIN
1e6b565 
The parent's end of a task's output pipe is registered with uloop_fd_add(),
which puts the descriptor in non-blocking mode. readall() and
uc_uloop_pipe_receive_common() were written for a blocking descriptor:
they treat any read failure other than EINTR as fatal and jump to
read_fail, discarding the bytes already consumed.

So a message that does not arrive in one go is lost, and worse, the stream
is left misaligned. There is no partial-message state, so the next
readable event re-enters part way through the payload and reads eight
bytes of content as the next length header, after which nothing on the
channel is interpretable.

Measured against a task sending a 100 KB payload, a sentinel and a return
value: 11 of 25 runs lose messages. At 4 MB, one of the three arrives.

Register the descriptor with ULOOP_BLOCKING so the reader's assumption
holds. Both runs then deliver 25 of 25 and 3 of 3. A child that dies mid
message still terminates the read, since the write end closing gives EOF
rather than a stall.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
fs: report a write that only fails when the file is closed
6e1592a 
fwrite() fills the stdio buffer, so for any payload smaller than that
buffer the real write(2) happens inside fclose(). Both writefile() and
file.close() discard fclose()'s return value, so ENOSPC, EIO and EDQUOT
there are lost and the call reports success.

  import * as fs from 'fs';

  fs.writefile('/dev/full', 'hello world!!');

returns 13 with no error. That is the shape of essentially every config,
state and sysfs write, and on a full overlay, an ordinary OpenWrt failure,
the standard write-to-temp-then-rename save reports success, commits the
rename, and replaces a good config with an empty file.

flush() already surfaces the same errno, which is how we know it is
available and simply being dropped.

Check fclose() in both, preferring an error fwrite() already reported.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
fs: keep the bytes a short read already delivered
d075095 
uc_fs_read_common() throws away everything fread() returned whenever the
error indicator is set, so a read that is satisfied in part loses the part
it got. The bytes are gone from the pipe as well, so nothing can recover
them.

Arming uloop.handle() on a handle puts the descriptor in non-blocking
mode, which is what libubox does unless ULOOP_BLOCKING is passed, and that
is the documented way to read a handle from an event callback:

  import * as uloop from 'uloop';
  import * as fs from 'fs';

  uloop.init();
  let p = fs.pipe();
  uloop.handle(p[0], function(ev) {}, uloop.ULOOP_READ);
  p[1].write('PAYLOAD-A');
  p[1].flush();
  p[0].read(64);

returns null with EAGAIN, while strace shows read(9, "PAYLOAD-A", 4096) = 9.

Report the error only when nothing was read, and clear the indicator
either way: it is sticky, so leaving it set failed every later read on the
same handle.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
resolv: file each NS, CNAME and PTR record under its own type
8c01857 
parse_reply() shares one `key` variable across the whole answer loop and
the ns_t_ns / ns_t_cname / ns_t_ptr fall-through chain only assigns it
when it is still NULL. That is how the chain picks the right name for
whichever case was entered, but the variable is declared outside the loop,
so only the first such record chooses. Every later one is filed under the
first one's type.

A reply carrying a CNAME followed by a PTR is the stock RFC 2317 classless
reverse delegation, and it comes back with both entries under CNAME and no
PTR key at all, so a caller reading result[name].PTR sees null for a
lookup that succeeded.

Clear it at the top of each iteration.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
struct: decode any non-zero byte as true in native mode
4fec71d 
native_unpack_bool() memcpys the byte straight into a _Bool. Only 0 and 1
are valid representations of that type, so reading back a byte that holds
anything else is undefined, and the compiler is entitled to fold x != 0
into a test of the low bit alone.

  import * as struct from 'struct';

  struct.unpack('?',  '\xfe');   /* false */
  struct.unpack('<?', '\xfe');   /* true  */

The module documents that "any non-zero value will be true when
unpacking", and the standard-mode handler beside it reads *p != 0 and gets
it right, so only the native format is affected. A protocol flag byte of
0x02, 0x10, 0x80 or 0xFE decodes as false with nothing to indicate it.

Read the byte as a byte.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
zlib: inflate every member of a concatenated stream
edf5b32 
A gzip stream may be several members one after another, and that is what
gzip -c f1 f2, cat a.gz b.gz and pigz all produce. Both inflate paths
stopped at the first Z_STREAM_END: uc_zlib_inf_string() returned with
avail_in still set, and uc_zlib_inf_object() left its read loop. Nothing
in the module ever called inflateReset(), so every member after the first
was dropped with no error and no null.

  import * as zlib from 'zlib';
  import { open } from 'fs';

  zlib.inflate(open('two.gz', 'r'));

returns AAAA where gzip -dc, zcat and python all return AAAABBBB.

Reset the stream and carry on while input remains, on both paths. The
object path now also drains its source rather than stopping mid-file,
which is what its documented contract already describes: reading stops
when read() returns null or an empty string.

The assert() that would have caught the leftover input is compiled out by
the unconditional -DNDEBUG in CMakeLists.txt.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
resolv: release the TXT buffer when the record is malformed
b844c63 
Both TXT branches allocate before validating the per-string length inside
the record, and the length check then returns -1 without freeing what it
allocated. The array branch loses a ucv_array; the default branch loses a
ucv_stringbuf_new(), a raw json-c printbuf the VM does not track, so that
one and everything appended to it are definitely lost rather than merely
unreclaimed.

valgrind reports 1920 bytes definitely lost over 40 malformed replies,
against 0 over well-formed ones. The amount scales with the TXT content,
which the answering server chooses, so it is remote and unbounded.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
compiler: unwind the nested function compiler on a bad argument list
664b707 
uc_compiler_compile_funcexpr_common() sets up a second uc_compiler_t for
the function body, and uc_compiler_finish() is what releases its locals
and upvalue vectors, the values naming them, and the function object. The
"Expecting Label" path in the argument loop returns straight out instead,
so all of it leaks.

  let x = 1; function f( = 1; function f(; } print(f(x));

leaks 562 bytes in four allocations. The sibling error path a few lines
below, for a missing '{' after the parameters, already falls through to
the teardown, so make this one do the same: the syntax error is latched
either way, and uc_compiler_finish() frees the function rather than
returning it once parser->error is set.

Found by the new fuzz layer.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
struct: accumulate signed integers in an unsigned type
ab988ea 
The four signed integer unpackers build the value with x = (x<<8) | byte
into a signed variable. Once the byte that lands in the sign bit arrives
the shift overflows, which is undefined:

  import * as struct from 'struct';

  struct.unpack('!q', '\xff\xff\xff\xff\xff\xff\xff\xff');

UBSan reports "left shift of 41255877746768127 by 8 places cannot be
represented in type 'long long'". The unsigned unpackers beside them
already accumulate into an unsigned type and are unaffected, so the fix is
to make the signed ones match and convert once at the end. The sign
extension that follows is unchanged.

Values across the range decode as before, including INT64_MIN, INT64_MAX
and -1 in both byte orders.

Found by the new struct fuzz target.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
compiler: do not overflow int building the wildcard import operand
d996fad 
uc_compiler_compile_module_source() encodes a wildcard import as

  source->exports.count | (0xffff << 16)

and 0xffff is a plain int constant, so the shift produces 0xFFFF0000,
which does not fit in an int. Signed left-shift overflow is undefined, and
UBSan reports "left shift of 65535 by 16 places cannot be represented in
type 'int'".

The operand is emitted for every

  import * as name from "module.uc"

of a source module, so this is the ordinary path rather than a corner:
gdb stops on the line for a two-export module imported that way.

Make the constant unsigned. The value emitted is unchanged.

Found by the compile fuzz target, after 2.25 million executions.

Signed-off-by: John Crispin <john@phrozen.org>
@blogic @jow-
vm: give division by zero the right sign, and NaN for zero over zero
24df8d2 
Both arithmetic paths answer any division by zero with +Infinity, whatever
the numerator was:

  -1 / 0     Infinity, where every other language gives -Infinity
   0 / 0     Infinity, where the answer is NaN

The double path was special-casing a zero divisor to reach that; plain
division already produces the IEEE result, including a signed zero
divisor, so the case is removed rather than corrected. The integer path
has no hardware answer to fall back on and now picks between NaN and a
signed infinity itself.

Anything that divides by a computed denominator sees this. A quotient of
NaN and one of +Infinity behave differently in a comparison, so the wrong
one is not a cosmetic difference.

Signed-off-by: John Crispin <john@phrozen.org>
@jow-
jow- merged commit 416f707 into master Oct 3, 2026
3 checks passed
@jow-
jow- deleted the fix/blogic-fixes-1 branch October 3, 2026 16:09
@jow- jow- mentioned this pull request Oct 3, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jow- @blogic