Skip to content

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

@jeffrson

Description

@jeffrson

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions