mscdex / mscdex/ssh2

sftp.read callback receives wrong position value when request exceeds _maxReadLen

Open Beginner friendly
#1,488 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
5.8k
Forks
734
PR merge metrics
No merged PRs in 30d

Description

The fourth callback argument of sftp.read() (conventionally named position) returns a wrong value whenever the requested read length exceeds _maxReadLen internally. Instead of reflecting the position argument originally passed to sftp.read(), it returns the file offset of the last internal sub-read.

Reproducer

  sftp.read(handle, buf, 0, 32 * 1024, 0, (err, bytesRead, buf, position) => {
    console.log(position); // prints 31952 — expected: 0
  });

This triggers reliably against any non-OpenSSH server (also SSH2 based!) because those servers cause the client to set _maxReadLen = 31952 (= 34000 - PKT_RW_OVERHEAD). A request of 32 KB exceeds that threshold by 816 bytes.

Root cause

read_() in SFTP.js splits oversized requests into sequential sub-reads. req.position is used as a mutable cursor to track the offset of
each sub-read (line 2154):

  req.nb += nb;
  req.position += nb;   // ← mutates the original position
  req.off += nb;
  read_(self, handle, buf, req.off, overflow, req.position, cb, req);

At the end of the final sub-read, the mutated cursor value is passed to the user callback instead of the original position (line 2165):

  cb(undefined, req.nb + nb, data, req.position);  // ← wrong: should be original position

The same concern was already handled correctly for off via req.origOff. No equivalent origPosition is saved.

Why it is rarely seen with OpenSSH servers

When the remote server identifies as OpenSSH, the client sets _maxReadLen = 260096 (~254 KB). Typical SFTP clients read in chunks of ≤ 32
KB, so the overflow path is never triggered. With non-OpenSSH servers the threshold drops to 31952 bytes, making the bug trivially
reproducible with an ordinary 32 KB read — a very common chunk size in practice. With about 300 KB chunksize, the errorneous behaviour can be seen with OpenSSH as well.

Suggested fix

Save the original position once, keep req.position as the internal cursor, and pass the saved value to the final callback:

  const req = {
    nb: 0,
    position,
    origPosition: position,   // ← save original
    off,
    origOff: off,
    // ...
    cb: (err, data, nb) => {
      // ...
      cb(undefined, req.nb + nb, data, req.origPosition);  // ← use original
    }
  };

A previous attempt to fix this was made in #1171 (attached to an unrelated bug report). This is a standalone report with a complete root
cause analysis.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in SFTP.js at read_, especially the request handling around lines 2154 and 2165, and reproduce the issue with a 32 KB sftp.read call against a non-OpenSSH server. Confirm that internal sub-read tracking can advance while the callback still receives the original position; done means the callback reports the supplied position for oversized reads.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
networking
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.