Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid Zlib Instance uncatchable error #1178

Open
GabrielLomba opened this issue May 9, 2022 · 5 comments
Open

Invalid Zlib Instance uncatchable error #1178

GabrielLomba opened this issue May 9, 2022 · 5 comments

Comments

@GabrielLomba
Copy link

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:

Error: Invalid Zlib instance
    at Zlib.writeSync (.../ssh2-sftp-client/node_modules/ssh2/lib/protocol/zlib.js:73:13)
    at ZlibPacketWriter.finalize (.../ssh2-sftp-client/node_modules/ssh2/lib/protocol/zlib.js:190:33)
    at Protocol.channelData (.../ssh2-sftp-client/node_modules/ssh2/lib/protocol/Protocol.js:462:43)
    at tryWritePayload (.../ssh2-sftp-client/node_modules/ssh2/lib/protocol/SFTP.js:2523:22)
    at sendOrBuffer (.../ssh2-sftp-client/node_modules/ssh2/lib/protocol/SFTP.js:2491:15)
    at SFTP.close (.../ssh2-sftp-client/node_modules/ssh2/lib/protocol/SFTP.js:397:24)
    at onerror (.../ssh2-sftp-client/node_modules/ssh2/lib/protocol/SFTP.js:2098:13)
    at writeCb (.../ssh2-sftp-client/node_modules/ssh2/lib/protocol/SFTP.js:2178:22)
    at Object.cb (.../ssh2-sftp-client/node_modules/ssh2/lib/protocol/SFTP.js:478:13)
    at cleanupRequests (.../ssh2-sftp-client/node_modules/ssh2/lib/protocol/SFTP.js:2584:11)
    at SFTP.push (.../ssh2-sftp-client/node_modules/ssh2/lib/protocol/SFTP.js:191:7)
    at onCHANNEL_CLOSE (.../ssh2-sftp-client/node_modules/ssh2/lib/utils.js:50:13)
    at ChannelManager.cleanup (.../ssh2-sftp-client/node_modules/ssh2/lib/utils.js:200:7)
    at Socket.<anonymous> (.../ssh2-sftp-client/node_modules/ssh2/lib/client.js:769:21)
    at Socket.emit (events.js:400:28)
    at Socket.emit (domain.js:475:12)

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 close event here, we call proto.cleanup() (which nullifies the Zlib instance handle that will be checked later and throw) and this._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.

@mscdex
Copy link
Owner

mscdex commented Jun 11, 2022

Do you have a minimal example to reproduce this issue using just ssh2?

@GabrielLomba
Copy link
Author

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
Copy link
Author

@mscdex any plans to handle this behavior? We still experience it

@BitStream1
Copy link

I too have been experiencing this issue. The throw is coming from here:

throw new Error('Invalid Zlib instance');

_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:

  1. Copy/fork and modify zlib.js to simply return at this line instead of throw. (It's probably not great to throw if it can't be caught, otherwise it could eventually lead to an unhandled exception.)
  2. Set algorithms: { compress: ['none'] }. zlib.js is only used when compression is enabled.

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:

  1. this one
  2. publickey auth fails in ssh2, but works from command line #989.
  3. If I remember right, fastXfer() errors with 0 byte length source files (e.g. see if (fsize <= 0) return onerror()). I also ended up writing a Parallel(Read|Write)Stream that keeps multiple read(fd,...) / write(fd,...)'s on the wire concurrently, reassembling reads sequentially and splitting into parallel writes. This means you can use the same parallelization strategy with pipes, which might be of interest.

Keep up the excellent work. With a few small adjustments it will be perfect!

@mscdex
Copy link
Owner

mscdex commented May 14, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants