kylefarris / clamscan

A robust ClamAV virus scanning library supporting scanning files, directories, and streams with local sockets, local/remote TCP, and local clamscan/clamdscan binaries (with failover).
MIT License
236 stars 69 forks source link

No error when server unavailable #51

Closed jobachhu closed 4 years ago

jobachhu commented 4 years ago

Hi,

I'm setting up clamscan to work with a clamd via a TCP socket using the passthrough() stream to check files that are uploaded. It's working fine but while I was testing different error conditions, I was unable to handle the error when the clamd server is not available. No error is thrown, it just logs a message and then waits until it times out.

This is the log message:

node-clam: Error initiating socket to ClamAV:  { Error: connect ECONNREFUSED 127.0.0.1:3310
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1106:14)
  errno: 'ECONNREFUSED',
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 3310 }

Here's the basic setup (some parts omitted for brevity):

try {
  let clamScanPromise = new NodeClam().init({
    clamdscan: {
      host: opts.clamavHost, // 127.0.0.1
      port: opts.clamavPort // 3310
    }
  })

  const clamscan = await this.clamScanPromise
  let avStream = clamscan.passthrough();
  avStream.on('error', cb)
  avStream.on('scan-complete', result => {
    // not reached
  })

  let scanPromise = new Promise(resolve => {
    avStream.on('scan-complete', resolve)
  })
  file.stream.pipe(avStream)
  // omitted avStream being piped to s3
} catch (err) {
  // not reached
}

The error seems to originate here in the code and it doesn't look like this error is propagated out in any way. Is there any way to react to the error? It seems like clamscan learns of the error, but my code waits for a timeout. If the server is unavailable, I'd like to reject the request with an error status instead of waiting.

Thanks in advance for the help!

kylefarris commented 4 years ago

That's interesting. I'm now wondering if I accidentally left out some kind of error handling there or if there was a reason I didn't have it there (hard to test, etc...)? I just merged another PR in yesterday so I'll see if I can figure this out before creating a new patch that will include a fix for this as well as that PR merge.

Thanks for submitting the bug, BTW...

kylefarris commented 4 years ago

Actually, it looks like someone already put a PR (#39) in to fix this but I apparently missed it. I've merged the change in.

kylefarris commented 4 years ago

I released a new minor version on NPM: v1.3.0.

jobachhu commented 4 years ago

Thanks for the quick response! Just upgraded and it works like a charm 👍