mscdex
commented
2 years ago Do you have a minimal example to reproduce this issue using just ssh2?
Open GabrielLomba opened 2 years ago
mscdex
commented
2 years ago Do you have a minimal example to reproduce this issue using just ssh2?
GabrielLomba
commented
2 years ago Yes. The examples are below:
Server code (mostly adapted from the example SFTP server code)
const {timingSafeEqual} = require('crypto');
const {readFileSync} = require('fs');
const ssh2 = require('ssh2');
const {utils: {sftp: {OPEN_MODE, STATUS_CODE}}} = ssh2;
const allowedUser = Buffer.from('foo');
const allowedPassword = Buffer.from('bar');
function checkValue(input, allowed){
const autoReject = input.length !== allowed.length;
if (autoReject)
allowed = input;
const isMatch = timingSafeEqual(input, allowed);
return !autoReject && isMatch;
}
new ssh2.Server({hostKeys: [readFileSync('ssh_bug_key')]}, client=>{
console.log('Client connected!');
client.on('authentication', ctx=>{
let allowed = true;
if (!checkValue(Buffer.from(ctx.username), allowedUser))
allowed = false;
switch (ctx.method)
{
case 'password':
if (!checkValue(Buffer.from(ctx.password), allowedPassword))
return ctx.reject();
break;
default:
return ctx.reject();
}
if (allowed)
ctx.accept();
else
ctx.reject();
}).on('ready', ()=>{
console.log('Client authenticated!');
client.on('session', (accept, reject)=>{
const session = accept();
session.on('sftp', (_accept, _reject)=>{
console.log('Client SFTP session');
const openFiles = new Map();
let handleCount = 0;
const sftp = _accept();
sftp.on('OPEN', (reqid, filename, flags, attrs)=>{
if (!filename.startsWith('/tmp/ssh_bug') || !(flags & OPEN_MODE.WRITE))
return sftp.status(reqid, STATUS_CODE.FAILURE);
const handle = Buffer.alloc(4);
openFiles.set(handleCount, true);
handle.writeUInt32BE(handleCount++, 0);
console.log('Opening file for write');
sftp.handle(reqid, handle);
}).on('WRITE', (reqid, handle, offset, data)=>{
if (handle.length !== 4 || !openFiles.has(handle.readUInt32BE(0)))
return sftp.status(reqid, STATUS_CODE.FAILURE);
// Fake the write operation
sftp.status(reqid, STATUS_CODE.OK);
console.log('Write to file at offset ${offset}: ${inspect(data)}');
})
.on('CLOSE', (reqid, handle)=>{
let fnum;
if (handle.length !== 4 || !openFiles.has(fnum = handle.readUInt32BE(0)))
return sftp.status(reqid, STATUS_CODE.FAILURE);
console.log('Closing file');
openFiles.delete(fnum);
sftp.status(reqid, STATUS_CODE.OK);
});
});
});
}).on('close', ()=>console.log('Client disconnected'));
}).listen(40000, '127.0.0.1', ()=>console.log('Listening on port 40000'));Client code
const ssh2 = require('ssh2');
const conn = new ssh2.Client();
conn.on('ready', ()=>{
console.log('Client: ready');
conn.sftp((err, sftp)=>{
if (err)
return console.log('SFTP conn err: ', err);
let n_uploads = 0, parallel = 5, upload_more = true;
let closing = false, local_file = '/tmp/ssh_test_file.txt';
let upload = ()=>{
if (!upload_more)
{
if (!closing)
{
closing = true;
console.log('Ending connection');
conn.end();
}
return;
}
sftp.fastPut(local_file, `/tmp/ssh_bug_${n_uploads++}.txt`, null, _err=>{
if (_err)
{
setTimeout(()=>console.log('Exiting'), 5000);
return console.log('Fast put err: ', _err);
}
if (n_uploads%100==0)
console.log('Uploaded: ', n_uploads);
upload();
});
};
for (let i = 0; i < parallel; i++)
upload();
setTimeout(()=>upload_more = false, 1000);
});
})
.on('error', e=>console.log('Got client error: ', e))
.connect({
host: '127.0.0.1',
port: 40000,
username: 'foo',
password: 'bar',
algorithms: {compress: ['zlib', 'zlib@openssh.com', 'none']},
}).on('close', ()=>console.log('Connection closed'));
// prevents Node process from exiting unless we have an uncaught exception,
// which happens
process.stdin.resume();Note that /tmp/ssh_test_file.txt was a sample 1MB file. You'll face this error often.
GabrielLomba
commented
1 year ago @mscdex any plans to handle this behavior? We still experience it
BitStream1
commented
1 year ago I too have been experiencing this issue. The throw is coming from here: https://github.com/mscdex/ssh2/blob/50ffab8faf473ee94078105d5d8af2dad1c91f0d/lib/protocol/zlib.js#L73
_close() sets handle to null. If there's a pending write that gets flushed after closing, it will trigger this throw. I don't think it can be caught by an outside exception handler.
2 work-arounds I've used in the past are:
Overall, this is a really, really great SSH library. I've used pretty much every capability it has in my current project.
There are only about 3 issues that I've had to work around:
Keep up the excellent work. With a few small adjustments it will be perfect!
mscdex
commented
1 year ago I haven't had time to look at this particular issue. If anyone feels up to it, feel free to submit a PR because I cannot guarantee when I will get a chance to work on it. If you do though, please make sure to include a test case that is as minimal as possible (preferably without touching the local disk).
I'm currently using this module through the ssh2-sftp-client module (using the latest version, v8.0.0) and ran across this sporadic error:
It looks like this lib is not gracefully handling disconnections with pending SFTP requests. In our current usage, we'll send a few files in parallel and if we get an error somehow (SFTP related or not), we'll end the client via SFTP client end() method. In its first appearance, I started waiting for the currently running fastPut/put calls before propagating the error expecting this issue would be gone but it still happens.
After checking the related code and following the stack trace, I figured it may be related to how the lib handles disconnections in its pending requests. In the client.end() method, we end its internal socket and in its
closeevent here, we callproto.cleanup()(which nullifies the Zlib instance handle that will be checked later and throw) andthis._chanMgr.cleanup(err)afterwards, which will cleanup pending requests with errors, some of which will (incorrectly) try to write data after the connection has been closed, relying on the (now null) handle and throwing this error.I'd expect the lib would just disregard the remaining read/write requests as it's an intended disconnection and move on but we end up with this uncaught error instead. If I'm missing something and this is the intended behavior, having a way to catch and handle this error would be appreciated.