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

Discard stderr messages when using offline sniff #2558

Merged
merged 1 commit into from
May 22, 2020

Conversation

roykuper13
Copy link
Contributor

@roykuper13 roykuper13 commented Mar 30, 2020

I'm opening this PR, because in my project i'm doing a lot of offline sniffing (mostly for filtering purposes). each time i'm using offline feature of sniff I get tcpdump's stderr, which is very very annoying to me, and for the user who uses my project.

I didn't make quiet an option in sniff for two reasons:

  1. I don't think anyone would want to see this reading from file -, link-type EN10MB (Ethernet) message from tcpdump. it's not indicative and tells the user nothing. it's just very disturbing.
  2. this PR affects only the offline sniff outputs, not other features of sniff, so putting quiet flag in sniff just doesn't make sense.

i'll be glad if you'll merge this, or to get suggestion for other solutions!
Thanks

@codecov
Copy link

codecov bot commented Mar 30, 2020

Codecov Report

Merging #2558 into master will decrease coverage by 0.34%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2558      +/-   ##
==========================================
- Coverage   88.28%   87.93%   -0.35%     
==========================================
  Files         243      246       +3     
  Lines       51006    52790    +1784     
==========================================
+ Hits        45032    46423    +1391     
- Misses       5974     6367     +393     
Impacted Files Coverage Δ
scapy/sendrecv.py 85.99% <ø> (ø)
scapy/layers/tls/automaton_cli.py 67.23% <0.00%> (-16.54%) ⬇️
scapy/layers/tls/automaton_srv.py 66.39% <0.00%> (-11.77%) ⬇️
scapy/compat.py 74.69% <0.00%> (-3.51%) ⬇️
scapy/layers/tls/record_tls13.py 81.76% <0.00%> (-1.58%) ⬇️
scapy/arch/windows/__init__.py 70.58% <0.00%> (-1.55%) ⬇️
scapy/layers/netflow.py 88.51% <0.00%> (-0.82%) ⬇️
scapy/route.py 90.51% <0.00%> (-0.73%) ⬇️
scapy/main.py 72.80% <0.00%> (-0.41%) ⬇️
scapy/data.py 92.17% <0.00%> (-0.35%) ⬇️
... and 32 more

@roykuper13
Copy link
Contributor Author

roykuper13 commented Mar 30, 2020

I'm not sure how debug the failure in travis pipline.
For example, in job #8670.2, All tests have FAILED=0 and in the end
___________________________________ summary ____________________________________ pypy-linux_root: commands succeeded codecov: commands succeeded congratulations :) ERROR: Too many parameters : [/tmp/tmp.b2K0h2nmPJ] The command "bash .config/ci/test.sh" exited with 1.

So i assume I have no failure, and there's a generic problem related to the travis.yml file?

@roykuper13 roykuper13 force-pushed the offline-sniff-quiet-stderr branch 3 times, most recently from d3c4502 to dbdfca3 Compare March 30, 2020 16:14
@gpotter2
Copy link
Member

Yes. We very recently migrated our test infrastructure, this issue is being tracked #2554

@guedou
Copy link
Member

guedou commented Mar 31, 2020

I am fine with defaulting to True for the tcpdump() quiet option, but I want to a have sniff() accept the quiet option as file. This will simplify debugging tcpdump errors. Thanks.

@roykuper13
Copy link
Contributor Author

roykuper13 commented Apr 9, 2020

I am fine with defaulting to True for the tcpdump() quiet option, but I want to a have sniff() accept the quiet option as file. This will simplify debugging tcpdump errors. Thanks.

@guedou
alright, it's done, you can check my recent changes 👍

@guedou
Copy link
Member

guedou commented Apr 10, 2020

It looks good to me. @p-l- @gpotter2 what it is your call?

@guedou guedou requested review from gpotter2 and p-l- April 10, 2020 12:08
@gpotter2
Copy link
Member

gpotter2 commented Apr 10, 2020

I fail to see what the use case of debug_file would be. Who would ever redirect it to a file ? This sounds like a totally unnecessary change, and breaks an API that has been around for three years 2a8b96a

I'd rather just have quiet a default

@p-l-
Copy link
Member

p-l- commented Apr 11, 2020

An option to have tcpdump's stderr sent to stderr would be enough IMO

@roykuper13
Copy link
Contributor Author

roykuper13 commented Apr 12, 2020

I fail to see what the use case of debug_file would be. Who would ever redirect it to a file ? This sounds like a totally unnecessary change, and breaks an API that has been around for three years 2a8b96a

I'd rather just have quiet a default

An option to have tcpdump's stderr sent to stderr would be enough IMO

@gpotter2 @p-l-
Agree with you guys. Added a quiet option to sniff, so now when set to True, tcpdump's stderr will be discarded 👍
Pushed my changes, so you can now review it.

@gpotter2
Copy link
Member

gpotter2 commented Apr 12, 2020

IMO the best solution would be to always discard it except when the sub process returncode is 1.
This would need to pipe the stderr, retrieve during communicate(), and print it only on failure.

@roykuper13
Copy link
Contributor Author

IMO the best solution would be to always discard it except when the sub process returncode is 1.
This would need to pipe the stderr to a buffer

@gpotter2 @p-l-
So i'm confused because you guys suggest different things.. what should I do then?
I quite support p-l-'s one to be honest.

@guedou
Copy link
Member

guedou commented Apr 12, 2020

I am fine with the current proposal. Relying on tcpdump is complicated as it does not have the same behavior across all platforms that we support.

@p-l-
Copy link
Member

p-l- commented Apr 12, 2020

I am fine with the current proposal. Relying on tcpdump is complicated as it does not have the same behavior across all platforms that we support.

Totally, that's why I suggest to let tcpdump stderr go to scapy's stderr if the user wants so (by setting debug=True)

@roykuper13
Copy link
Contributor Author

I am fine with the current proposal. Relying on tcpdump is complicated as it does not have the same behavior across all platforms that we support.

Totally, that's why I suggest to let tcpdump stderr go to scapy's stderr if the user wants so (by setting debug=True)

@guedou @p-l-
updated my changes, so now the user can "quiet" tcpdump's stderr if he wants. You can review it :)

Copy link
Member

@guedou guedou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@guedou guedou merged commit c72d229 into secdev:master May 22, 2020
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

Successfully merging this pull request may close these issues.

4 participants