Skip to content

Commit

Permalink
lib: fix read() overflow handling
Browse files Browse the repository at this point in the history
  • Loading branch information
@mscdex
mscdex committed Apr 25, 2021
1 parent d540e2a commit cda045a
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 53 deletions.
135 changes: 84 additions & 51 deletions lib/protocol/SFTP.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -406,57 +406,7 @@ class SFTP extends EventEmitter {
if (position === null)
throw new Error('null position currently unsupported');

const maxDataLen = this._maxPktLen - PKT_RW_OVERHEAD;
const overflow = Math.max(len - maxDataLen, 0);
const origPosition = position;

if (overflow)
len = maxDataLen;

/*
uint32 id
string handle
uint64 offset
uint32 len
*/
const handleLen = handle.length;
let p = 9;
let pos = position;
const out = Buffer.allocUnsafe(4 + 1 + 4 + 4 + handleLen + 8 + 4);

writeUInt32BE(out, out.length - 4, 0);
out[4] = REQUEST.READ;
const reqid = this._writeReqid = (this._writeReqid + 1) & MAX_REQID;
writeUInt32BE(out, reqid, 5);

writeUInt32BE(out, handleLen, p);
out.set(handle, p += 4);
p += handleLen;
for (let i = 7; i >= 0; --i) {
out[p + i] = pos & 0xFF;
pos /= 256;
}
writeUInt32BE(out, len, p += 8);

this._requests[reqid] = {
cb: (err, data, nb) => {
if (typeof cb !== 'function')
return;
if (err) {
if (cb._wantEOFError || err.code !== STATUS_CODE.EOF)
return cb(err);
} else if (nb > len) {
return cb(new Error('Received more data than requested'));
}
cb(undefined, nb || 0, data, position);
},
buffer: bufferSlice(buf, off, off + len),
};

const isBuffered = sendOrBuffer(this, out);
this._debug && this._debug(
`SFTP: Outbound: ${isBuffered ? 'Buffered' : 'Sending'} READ`
);
read_(this, handle, buf, off, len, position, cb);
}
readData(handle, buf, off, len, position, cb) {
// Backwards compatibility
Expand Down Expand Up @@ -1811,6 +1761,89 @@ function tryCreateBuffer(size) {
}
}

function read_(self, handle, buf, off, len, position, cb, req_) {
const maxDataLen = self._maxPktLen - PKT_RW_OVERHEAD;
const overflow = Math.max(len - maxDataLen, 0);

if (overflow)
len = maxDataLen;

/*
uint32 id
string handle
uint64 offset
uint32 len
*/
const handleLen = handle.length;
let p = 9;
let pos = position;
const out = Buffer.allocUnsafe(4 + 1 + 4 + 4 + handleLen + 8 + 4);

writeUInt32BE(out, out.length - 4, 0);
out[4] = REQUEST.READ;
const reqid = self._writeReqid = (self._writeReqid + 1) & MAX_REQID;
writeUInt32BE(out, reqid, 5);

writeUInt32BE(out, handleLen, p);
out.set(handle, p += 4);
p += handleLen;
for (let i = 7; i >= 0; --i) {
out[p + i] = pos & 0xFF;
pos /= 256;
}
writeUInt32BE(out, len, p += 8);

if (typeof cb !== 'function')
cb = noop;

const req = (req_ || {
nb: 0,
position,
off,
origOff: off,
len: undefined,
overflow: undefined,
cb: (err, data, nb) => {
let len = req.len;
const overflow = req.overflow;

if (err) {
if (cb._wantEOFError || err.code !== STATUS_CODE.EOF)
return cb(err);
} else if (nb > len) {
return cb(new Error('Received more data than requested'));
} else if (nb === len && overflow) {
req.nb += nb;
req.position += nb;
req.off += nb;
read_(self, handle, buf, req.off, overflow, req.position, cb, req);
return;
}

if (req.origOff === 0 && buf.length === req.nb)
data = buf;
else
data = bufferSlice(buf, req.origOff, req.origOff + req.nb);
cb(undefined, req.nb + (nb || 0), data, req.position);
},
buffer: undefined,
});

req.len = len;
req.overflow = overflow;

// TODO: avoid creating multiple buffer slices when we need to re-call read_()
// because of overflow
req.buffer = bufferSlice(buf, off, off + len);

self._requests[reqid] = req;

const isBuffered = sendOrBuffer(self, out);
self._debug && self._debug(
`SFTP: Outbound: ${isBuffered ? 'Buffered' : 'Sending'} READ`
);
}

function fastXfer(src, dst, srcPath, dstPath, opts, cb) {
let concurrency = 64;
let chunkSize = 32768;
Expand Down
29 changes: 27 additions & 2 deletions test/test-sftp.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,30 @@ setup('read', mustCall((client, server) => {
}));
}));

setup('read (overflow)', mustCall((client, server) => {
const maxChunk = 34000 - 2048;
const expected = Buffer.alloc(3 * maxChunk, 'Q');
const handle_ = Buffer.from('node.js');
const buf = Buffer.alloc(expected.length, 0);
let reqs = 0;
server.on('READ', mustCall((id, handle, offset, len) => {
++reqs;
assert.strictEqual(id, reqs - 1, `Wrong request id: ${id}`);
assert.deepStrictEqual(handle, handle_, 'handle mismatch');
assert.strictEqual(offset,
(reqs - 1) * maxChunk,
`Wrong read offset: ${offset}`);
server.data(id, expected.slice(offset, offset + len));
if (reqs === 3)
server.end();
}, 3));
client.read(handle_, buf, 0, buf.length, 0, mustCall((err, nb) => {
assert(!err, `Unexpected read() error: ${err}`);
assert.deepStrictEqual(buf, expected);
assert.strictEqual(nb, buf.length, 'read nb mismatch');
}));
}));

setup('write', mustCall((client, server) => {
const handle_ = Buffer.from('node.js');
const buf = Buffer.from('node.jsnode.jsnode.jsnode.jsnode.jsnode.js');
Expand All @@ -82,15 +106,16 @@ setup('write', mustCall((client, server) => {
}));

setup('write (overflow)', mustCall((client, server) => {
const maxChunk = 34000 - 2048;
const handle_ = Buffer.from('node.js');
const buf = Buffer.allocUnsafe(3 * 32 * 1024);
const buf = Buffer.allocUnsafe(3 * maxChunk);
let reqs = 0;
server.on('WRITE', mustCall((id, handle, offset, data) => {
++reqs;
assert.strictEqual(id, reqs - 1, `Wrong request id: ${id}`);
assert.deepStrictEqual(handle, handle_, 'handle mismatch');
assert.strictEqual(offset,
(reqs - 1) * 32 * 1024,
(reqs - 1) * maxChunk,
`Wrong write offset: ${offset}`);
assert((offset + data.length) <= buf.length, 'bad offset');
assert.deepStrictEqual(data,
Expand Down

0 comments on commit cda045a

Please sign in to comment.