search

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
230 stars 68 forks source link

Fix the problem reported in #102 #105

Closed benzino77 closed 5 months ago

benzino77 commented 2 years ago

Proper error handling in scanStream method.

benzino77 commented 2 years ago

hmmm... that is strange. On my side all tests are passing without errors...

kylefarris commented 2 years ago

Sometimes I have to run the CI tests a few times to get them to pass. Just something weird with the environment it runs in, I think.

benzino77 commented 2 years ago

There is a problem with node version, I think. On my side (on my workstation) tests run on node v17 sometimes failing:

Now using node v17.9.1 (npm v8.11.0)
❯ npm run test

> clamscan@2.1.2 test
> make test

npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR!
npm ERR! While resolving: eslint-config-airbnb@15.1.0
npm ERR! Found: eslint@7.25.0
npm ERR! node_modules/eslint
npm ERR!   dev eslint@"^7.25.0" from the root project
npm ERR!   peer eslint@">= 4.12.1" from babel-eslint@10.1.0
npm ERR!   node_modules/babel-eslint
npm ERR!     dev babel-eslint@"^10.1.0" from the root project
npm ERR!   5 more (eslint-config-prettier, eslint-plugin-chai-friendly, ...)
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer eslint@"^3.19.0 || ^4.3.0" from eslint-config-airbnb@15.1.0
npm ERR! node_modules/eslint-config-airbnb
npm ERR!   dev eslint-config-airbnb@"^15.0.1" from the root project
npm ERR!
npm ERR! Conflicting peer dependency: eslint@4.19.1
npm ERR! node_modules/eslint
npm ERR!   peer eslint@"^3.19.0 || ^4.3.0" from eslint-config-airbnb@15.1.0
npm ERR!   node_modules/eslint-config-airbnb
npm ERR!     dev eslint-config-airbnb@"^15.0.1" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR!
npm ERR! See /home/benzino/.npm/eresolve-report.txt for a full report.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/benzino/.npm/_logs/2022-08-11T08_57_29_795Z-debug-0.log
make: *** [Makefile:5: all] Error 1

on all other node versions (v12, v13, v14, v15, v16) I'm not observing such behavior.

benzino77 commented 2 years ago

Kyle are you want me to do something more with this?

carboneater commented 1 year ago

@benzino77 the error you pasted is related to npm 8, where it now enforces peer dependencies strictly

carboneater commented 1 year ago

@benzino77 the error you pasted is related to npm 8, where it now enforces peer dependencies strictly

Good news is I gnawed at this problem over the last few days. It's solved in #106

benzino77 commented 1 year ago

@benzino77 the error you pasted is related to npm 8, where it now enforces peer dependencies strictly

Good news is I gnawed at this problem over the last few days. It's solved in #106

That is great! So if your PR will be merged then there is a chance that this one will not fail on tests.

benzino77 commented 1 year ago

@kylefarris since #106 was merged could you please rerun tests for this PR? I have no idea how should I do it on my own....

origooo commented 6 months ago

Not sure what the status of this PR is, but it seems to fix the issues with error handling for me. Without this fix, sometimes the EPIPE error is not catchable which breaks my api endpoints somewhat. With the fix it is indeed properly caught.

Anything I can do to help?

kylefarris commented 6 months ago

Hey guys, I'll take a look at this today and run the tests and get it merged in if all is well. Sorry for the delay! Been really busy with work :)

origooo commented 6 months ago

Some more information. I've added a test case which passes a too large stream through scanStream (which was my initial problem). The fix suggested in this PR helps in catching the two types of errors which can occur from that. The errors are either EPIPE or NodeClamError "INSTREAM size limit exceeded".

What I don't understand is that the type of error I end up catching is inconsistent. This is okay since I can look for both types of errors. But do we know where the inconsistency comes from? It would be sweet if we could look for e.g. a "SIZE_LIMIT_EXCEEDED" or so but that would not be possible without fixing the inconsistency. I have seen that you've mentioned in other issue threads that we can look for NodeClamError + data.error.includes("INSTREAM size limit exceeded"). That might be better instead of adding specific error handling for all potential ClamAV errors.

EDIT I guess the answer to the inconsistency is found in the discussion leading up to this PR.

So to summarise/answer my comment above, we cannot have a "SIZE_LIMIT_EXCEEDED" on streams due to the answer in the link. This PR does, however, fix the error handling of too large streams.

kylefarris commented 5 months ago

Merged! Thanks everyone! I'm going to get dependencies updated and a new version published shortly.