yongtang / clamav.js

A node.js library for ClamAV
MIT License
27 stars 8 forks source link

Why don't you use the callback on socket.complete? #6

Open corybill opened 7 years ago

corybill commented 7 years ago

Hello, I'm testing out using your node module to send streams to clamav. Sometimes it looks like the "data" event doesn't get called. But since your code doesn't use the callback for the "close" event on the socket, it just hangs and never completes the request. On the requests that do not hit the "data" event, I can see the stream being successfully scanned in the clamav logs. I can provide clamav logs and configuration if that is helpful. Thanks.

I was wondering if this happens on very small files. So I increased the size of the file to over 500k and I still saw the same results.

Next I was wondering if it might happen when clamav uses its cache to determine that a file has already been scanned. So I changed the DisableCache configuration to 'yes' and still saw the same thing.

I'm wondering if you are doing this for a specific reason or if it is a bug?

To help you I have uploaded the file that seems to hit the "data" event sometimes, but other times it just goes directly to the "close" event.

Thanks for your help!

test-file.txt

corybill commented 7 years ago

After doing some packet sniffing what I found was that node js seems to be automatically sending the FIN tcp packet on the socket immediately after sending the last byte of data from the stream. As soon as Node sends that FIN packet, it's creating a race condition in the Clamav server. It is racing between finishing the scan / sending the response, and processing the FIN packet sent by Node and closing the connection.

This only happens when you pipe a stream into a socket and I have only tested on node 6.2.1.

For my purposes I am already using multipart-form/data so I already have a buffer with the binary file data. So I am just doing a socket.write(buffer) and allowing the clamav to send the FIN packet themselves after they send the response.

Definitely curious to hear if you guys have seen this issue though. I am still looking for a way to tell node not to send the FIN packet after sending the entire stream into socket, allowing the other side of the socket a chance to respond before killing the connection.

gabegorelick commented 6 years ago

After doing some packet sniffing what I found was that node js seems to be automatically sending the FIN tcp packet on the socket immediately after sending the last byte of data from the stream.

See https://nodejs.org/api/net.html#net_event_end. I think you want allowHalfOpen: true on the socket.

nalbion commented 6 years ago

@corybill were you able to find a solution for this? I've tried several modifications, but sometimes I can't reproduce the problem at all, which makes it really hard to test if a modification fixes the problem.

I don't suppose you are able to consistently reproduce the problem in a unit test?

nalbion commented 6 years ago

@gabegorelick allowHalfOpen: true does not fix the problem for me.

nalbion commented 6 years ago

Is ClamAV intended to support multiple nINSTREAM requests on the same socket from different hosts at the same time?

I'm using this library from AWS Lambda. If a user (or even more than one user) uploads multiple files to my system at the same time, I spin up multiple Lambda containers to scan the files (a Lambda container only serves one request at a time)

If 3 containers start streaming files on port 3310 as shown below

| Lambda 1  | Lambda 2  | Lambda 3  |
+-----------+-----------+-----------+
| nINSTREAM |           |           |
| streaming | nINSTREAM |           |
| streaming | streaming | nINSTREAM |
| streaming | streaming | streaming |
| EOF       | streaming | streaming |
|           | EOF       | streaming |
|           |           | EOF       |

...my expectation is that each connection would have its own thread and ephemeral response socket, and an EOF (zero-length chunk) on one connection would not affect the other connections. After restricting Lambda to only allow 1 container at a time I did not experience any problem (mind you, I did not run the test for very long). I'm starting to suspect that ClamAV might not support mutiple socket connections concurrently.

nalbion commented 6 years ago

I've raised an issue on the ClamAV system: https://bugzilla.clamav.net/show_bug.cgi?id=12181

corybill commented 6 years ago

It has been a while here but my solution was to use "socket.write(buffer)" instead of piping to a stream. The issue I saw that the fin packet was being sent automatically.

nalbion commented 6 years ago

@corybill I tried replacing with:

var socket = new net.Socket({allowHalfOpen: true});
  var status = '';
  socket.connect(port, host, function() {
//    stream.pipe(channel).pipe(socket)
    socket.write("nINSTREAM\n");
    stream.on('data', function(chunk) {
        var size = new Buffer(4);
        size.writeInt32BE(chunk.length, 0);
        socket.write(size);
        socket.write(chunk);
    }).on('end', function() {
        var size = new Buffer(4);
        size.writeInt32BE(0, 0);
        socket.write(size);

        complete(stream);
    })

...it seems to be a bit more stable, but I still see the process occasionally being terminated abruptly before getting a data event on the socket.

nalbion commented 6 years ago

I've logged a bug on the ClamAV system: https://bugzilla.clamav.net/show_bug.cgi?id=12181

I've also posted a question on StackOverflow: https://stackoverflow.com/questions/52072840/intermittent-issues-with-clamav-clamd-instream-on-socket

olemoign commented 4 years ago

In case anybody is still interested. 😄

If feel like replacing l41 with stream.pipe(channel).pipe(socket, { end: false }).on('end', function() { prevents sending the FIN packet as soon as the write is finished on the socket, allowing to wait for the clamav reponse.