Stop wPread and wPwrite dropping the high offset word

[yosuke-wolfssl]

Problem

wPread() / wPwrite() receive the SFTP file offset split into two 32-bit words, low word first — src/wolfsftp.c parses it that way and propagates carry into the high word. Two of the POSIX ports discarded that high word:

ret= (int)lseek(fd, shortOffset[0], SEEK_SET); /* high word dropped */

• The lseek fallback, compiled when the platform has no pread/pwrite, seeked with the low word alone. An SFTP read or write at or past 4 GiB silently hit a masked position: wrong data returned to the client on read, corruption on write.
• The native pread/pwrite pair combined the high word only under SIZEOF_OFF_T == 8, so a target with a 32-bit off_t truncated the same way.
• Separately, (int)lseek(...) narrowed the returned position before comparing it against -1, so a valid seek to 0xFFFFFFFF was misread as an error and the transfer was skipped.

Fix (src/port.c, wolfssh/port.h)

New wResolveOffset() assembles the split offset in word64 and rejects anything above WOLFSSH_MAX_FILE_OFFSET — the widest value the seek call of the port can take, 2^63-1 for a 64-bit off_t and 2^31-1 otherwise. All four POSIX helpers reduce to that guard plus a cast, so the SIZEOF_OFF_T split disappears from each of them. Assembling in an unsigned type removes the signed-shift overflow, and the narrow ceiling also rejects a 2–4 GiB offset whose high word is zero. lseek is compared against (off_t)-1 before any narrowing. A rejected offset surfaces as WOLFSSH_FTP_FAILURE for that one request; the session stays up.

Scope: this covers the two POSIX ports. Harmony, Zephyr, Nucleus and the fseek fallback still seek with the low word alone, and FATFS's ff_pread/ff_pwrite take no offset argument at all. The helper is port-neutral, so each can adopt it by defining its own WOLFSSH_MAX_FILE_OFFSET. f-8823 stays open for those.

Tests (tests/unit.c, .github/workflows/os-check.yml)

test_PreadPwriteHighOffset() drives an offset of exactly 4 GiB: wPread() must report EOF past a 16-byte file — a truncating port rereads the start and returns data — and wPwrite() must leave st_size at 4 GiB + 1. The write half is skipped on a file system that fills holes, detected by a 1 MiB probe, so it never materialises 4 GiB; only EFBIG/ENOSPC may skip. The truncation lives in the lseek port, which no CI job compiled, so os-check gains an --enable-all CFLAGS=-DWOLFSSH_LOCAL_PREAD_PWRITE entry — the first CI coverage of that branch. The temp file now honours TMPDIR.

Verification

• make check on --enable-all and on the new forced-fallback config: 11 pass, 1 skip, 0 fail.
• Negative control: reducing the helper to the low word turns the test red on the fallback build (10 pass, 1 fail). Pointing TMPDIR at a missing directory fails the test, confirming the path comes from the environment.
• Helper contract checked standalone under ASan + UBSan against both ceilings: 4 GiB and 2^63-1 accepted, 2^63 and above rejected; narrow ceiling accepts 0x7FFFFFFF and rejects 0x80000000 and 3 GiB. No UB now that the assembly is unsigned.
• gcc-13 -Werror clean across 6 configs, including the Zephyr define set.

[Copilot]

Pull request overview

This PR fixes incorrect handling of SFTP/SCP file offsets in the wPread() / wPwrite() portability layer, ensuring the high 32-bit word of a split offset is not silently dropped and that 32-bit off_t builds fail closed instead of truncating. It also adds a unit test that detects the 4 GiB truncation behavior regression.

Changes:

• Fix offset assembly in src/port.c for both the lseek()-fallback and native pread()/pwrite() paths; fail when off_t is too narrow instead of truncating.
• Fix lseek() error detection by comparing against (off_t)-1 before any narrowing.
• Add a unit test covering a 4 GiB offset to catch truncation regressions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/port.c	Correctly assembles 64-bit offsets (and fail-closed on narrow off_t) for wPread()/wPwrite() and fixes lseek() return handling.
tests/unit.c	Adds test_PreadPwriteHighOffset() to validate correct behavior at exactly 4 GiB offsets.

Suppressed comments (3)

src/port.c:187

• Same as in wPwrite(): assembling the 64-bit offset by left-shifting off_t can trigger undefined behavior for offsets >= 2^63. Build the combined offset in an unsigned 64-bit type and range-check before casting to off_t.

 #if SIZEOF_OFF_T == 8
offset = ((off_t)shortOffset[1] << 32) | offset;
#else
/* off_t cannot hold the high word, fail rather than truncate */
if (shortOffset[1] != 0)
return -1;

src/port.c:208

• The 64-bit offset assembly uses ((off_t)shortOffset[1] << 32), which left-shifts a signed type and can be undefined behavior for offsets >= 2^63. Consider building in word64 and rejecting values that don’t fit in signed off_t before calling pwrite().

 #if SIZEOF_OFF_T == 8
offset = ((off_t)shortOffset[1] << 32) | offset;
#else
/* off_t cannot hold the high word, fail rather than truncate */
if (shortOffset[1] != 0)
return -1;

src/port.c:224

• Same as in wPwrite(): building the offset with ((off_t)shortOffset[1] << 32) can be undefined behavior if the resulting signed off_t overflows (offsets >= 2^63). Build in word64 and range-check before casting to off_t for pread().

 #if SIZEOF_OFF_T == 8
offset = ((off_t)shortOffset[1] << 32) | offset;
#else
/* off_t cannot hold the high word, fail rather than truncate */
if (shortOffset[1] != 0)
return -1;

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1166

Scan targets checked:wolfssh-bugs, wolfssh-src

Findings: 6
6 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[ejohnstown]

The core fix is right, and catching the (int)lseek(...) narrowing as a second bug in the same helper is a good find -- a valid seek to 0xFFFFFFFF reading as an error is its own silent data-loss path.

I pulled the branch at 1fdb096 and reproduced your verification on macOS/APFS:

• --enable-all: make check 11 pass, 1 skip (external.test), 0 fail. PreadPwriteHighOffset ran the write half in full, so it really did materialise a 4 GiB + 1 sparse file and check st_size.
• --enable-all CFLAGS=-DWOLFSSH_LOCAL_PREAD_PWRITE: same result, no new compiler warnings.
• Negative control: reverting only the two lseek hunks and leaving everything else in place turns the test red. It does catch the bug it is written for, in the build that compiles that branch.

That last qualifier is the one thing I think should be settled before merge -- details inline on tests/unit.c. The other two inline notes are a correctness gap on narrow off_t and some duplication.

The other ports still drop the high word

Scope question rather than a defect in what you changed, and it spans several files so I could not pin it inline. This PR fixes the two POSIX ports, but the issue as titled -- wPread/wPwrite dropping the high offset word -- is still live in four others, all of which seek with shortOffset[0] alone:

• Harmony, src/port.c:129 and :142
• Zephyr, src/port.c:678 and :695
• Nucleus, wolfssh/port.h:824 and :838
• the fseek fallback, wolfssh/port.h:1183 and :1199

FATFS is worse: ff_pread/ff_pwrite (src/wolfsftp.c:2265, :2281) take no offset at all. Nucleus and the fseek fallback also only seek when ofst > 0, which assumes sequential access. Windows is fine -- RecvWrite/RecvRead set both OVERLAPPED.OffsetHigh and .Offset directly (src/wolfsftp.c:4410).

Fixing them all here would be a much larger change, so I would rather the PR body just said it covers the POSIX ports and that f-8823 stays open for the rest, instead of reading as if the class is closed. Harmony in particular is a one-liner away from consistency and was touched last week in 2a30f48.

[ejohnstown]

This is adjacent to Fenrir's high-word-coverage finding but a separate claim, so flagging it here rather than reopening that thread. Your reply there is right as far as it goes: breaking wPwrite's guard does turn the test red. But that shows the test catches a newly introduced rejection bug, not that it catches the truncation this PR removes.

I checked the latter directly: pre-PR src/port.c (255dd92) under this PR's tests/unit.c, default --enable-all, gives PreadPwriteHighOffset: SUCCESS. The test passes without the fix.

That follows from the code. On 64-bit the native pread/pwrite path already combined the high word correctly, so there was no truncation there to catch. The truncation this PR fixes lives in the WOLFSSH_LOCAL_PREAD_PWRITE branch, and nothing in .github/workflows/ sets that macro -- so no CI job compiles the code the test was written for.

Adding CFLAGS=-DWOLFSSH_LOCAL_PREAD_PWRITE to one os-check matrix entry would close it, and would give that whole branch its first CI coverage.

Minor, same function: tmpFile hardcodes /tmp and ignores TMPDIR. Worth honouring it, since a 4 GiB sparse file is a big thing to put somewhere the environment asked you not to.

[yosuke-wolfssl]

Both fixed. os-check gains '--enable-all CFLAGS=-DWOLFSSH_LOCAL_PREAD_PWRITE' — CFLAGS not CPPFLAGS, since the action's configure line already passes CPPFLAGS="-I...". First CI job to compile that branch. (The assignment costs nothing here: configure.ac:10 presets : ${CFLAGS=""}, so -O2 comes from AM_CFLAGS and survives.)

You were right that the test proved nothing where it ran. Negative control on the new config: reducing wResolveOffset() to the low word turns it red, 10 pass 1 fail.

TMPDIR honoured now, falling back to /tmp, following apps/wolfsshd/test/test_configuration.c:5593. Confirmed by pointing it at a missing directory — the test fails at mkstemp instead of quietly using /tmp.

[ejohnstown]

Different end of the range from Copilot's shift finding, which the > 0x7FFFFFFF guard has already handled.

The narrow-off_t arm checks the high word but not the low one, so the stated contract ("narrower: nonzero high word -> -1, otherwise correct") does not hold across the whole 32-bit range. With a 32-bit signed off_t and an offset in 2-4 GiB, shortOffset[1] is 0 and the guard passes, but (off_t)shortOffset[0] converts a value above 0x7FFFFFFF out of range. In practice two's-complement makes it negative and lseek/pread reject it with EINVAL, so it fails rather than corrupts -- which is the behavior you want, but it arrives by accident of the representation rather than by the guard.

Folding it into the same test makes the contract explicit:

if (shortOffset[1] !=0||shortOffset[0] >0x7FFFFFFF)
return-1;

Applies to all four helpers.

[yosuke-wolfssl]

Fixed, with the ceiling carrying it rather than a second clause. WOLFSSH_MAX_FILE_OFFSET is 0x7FFFFFFF when SIZEOF_OFF_T is 4, so a 2-4 GiB offset with a zero high word is rejected by the guard instead of surviving it and failing later in lseek with EINVAL. The contract holds by test now, not by representation.

Checked under UBSan: narrow ceiling accepts 0x7FFFFFFF, rejects 0x80000000 and 0xC0000000; wide accepts to 2^63-1 and rejects 2^63 up.

[ejohnstown]

This is the third of four copies of the same eight-line offset resolution. The two arms are mutually exclusive #elif branches, so a single file-scope static helper above the #if covers both and leaves one place to fix if the guard changes:

/* Resolve the split 64-bit SFTP offset. Returns -1 if off_t cannot hold it. */staticintwResolveOffset(constunsigned int*shortOffset, off_t*offset)

Each helper then reduces to a two-line guard plus the call. Worth doing here because the guard has already moved once in this PR, and it had to move in four places.

[yosuke-wolfssl]

Done, and made reusable by the other ports since they need the same fix. wResolveOffset() is a static inline in wolfssh/port.h, ahead of every port block including the ones defining wPread/wPwrite in the header itself.

It can't be typed on off_t — Nucleus seeks with INT32, fseek with long, Harmony int32_t, FATFS FSIZE_t — so it assembles into word64 and range-checks against a caller-supplied ceiling, leaving the caller to cast down. WOLFSSH_MAX_FILE_OFFSET is defined after the port chain under #ifndef, so a narrower port sets its own and the off_t default fills in otherwise. Each helper is now a guard plus a cast, and the #if SIZEOF_OFF_T split is gone from all four.

[yosuke-wolfssl]

Hello @ejohnstown ,
Thank you for reviewing. I reworked on this.
I'll take other ports as follow-ups.