Skip to content

fix bug for issue #1862, that is with new posts today. #2255

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 20, 2019
Merged

fix bug for issue #1862, that is with new posts today. #2255

merged 4 commits into from
Sep 20, 2019

Conversation

MaoJianwei
Copy link
Contributor

@MaoJianwei MaoJianwei commented Sep 18, 2019 •
edited
Loading

Improve the method to recognize tcpdump existence, covering more output cases in which tcpdump does not support '--version' option
Issue link:
#1862 (comment)

fixes #1862 with new posts today.

@MaoJianwei
fix bug for issue #1862
baefdf7 
Improve the method to recognize tcpdump existence, covering more output cases in which tcpdump does not support '--version' option
Issue link:
#1862 (comment)
@MaoJianwei MaoJianwei mentioned this pull request Sep 18, 2019
Closed
@MaoJianwei
Copy link
Contributor Author

Can we re-trigger the CI checking?

@guedou
Copy link
Member

guedou commented Sep 19, 2019

@MaoJianwei which OS/distribution are you using?

@MaoJianwei
Copy link
Contributor Author

MaoJianwei commented Sep 19, 2019 •
edited
Loading

Kernel 3.10，Red Hat 4.8 @guedou

@guedou
Copy link
Member

guedou commented Sep 19, 2019

4.8 or 5.8 ?

Could you share the full output of tcpdump --version? Thanks

@MaoJianwei
Copy link
Contributor Author

MaoJianwei commented Sep 19, 2019 •
edited
Loading

4.8 , I got it from cat /proc/version

the output is mentioned at #1862 by me, as follow:

tcpdump: unrecognized option '--version'
tcpdump version 4.5.1
libpcap version 1.5.3
Usage: tcpdump [-aAbdDefhHIJKlLnNOpqRStuUvxX] [ -B size ] [ -c count ]
                [ -C file_size ] [ -E algo:secret ] [ -F file ] [ -G seconds ]
                [ -i interface ] [ -j tstamptype ] [ -M secret ]
                [ -P in|out|inout ]
                [ -r file ] [ -s snaplen ] [ -T type ] [ -V file ] [ -w file ]
                [ -W filecount ] [ -y datalinktype ] [ -z command ]
                [ -Z user ] [ expression ]

And, this commit works for my server :) @guedou

guedou
guedou reviewed Sep 19, 2019
View reviewed changes
scapy/arch/common.py Outdated

# On some systems, --version does not exist on tcpdump
return proc.returncode == 0 or output.startswith(b'Usage: tcpdump ')
return proc.returncode == 0 or output.startswith(b'Usage: tcpdump ') or output.startswith(b'tcpdump: unrecognized option')
Copy link
Member

Choose a reason for hiding this comment

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

This line is longer than 79 characters. Please split it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have fixed this, please review again, thanks :)

@MaoJianwei
fix bug for issue #1862
9fdd12d 
Improve the method to recognize tcpdump existence, covering more output cases in which tcpdump does not support '--version' option
Issue link:
#1862
@codecov
Copy link

codecov bot commented Sep 19, 2019 •
edited
Loading

Codecov Report

Merging #2255 into master will decrease coverage by 0.68%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2255      +/-   ##
==========================================
- Coverage   85.62%   84.94%   -0.69%     
==========================================
  Files         235      235              
  Lines       49046    49144      +98     
==========================================
- Hits        41997    41746     -251     
- Misses       7049     7398     +349
Impacted Files Coverage Δ
scapy/arch/common.py 90.8% <100%> (ø) ⬆️
scapy/arch/bpf/supersocket.py 27.77% <0%> (-50.01%) ⬇️
scapy/layers/dhcp6.py 64.18% <0%> (-21.56%) ⬇️
scapy/contrib/openflow.py 78.39% <0%> (-10.42%) ⬇️
scapy/arch/bpf/core.py 81.37% <0%> (-5.89%) ⬇️
scapy/contrib/ethercat.py 93.83% <0%> (-4.33%) ⬇️
scapy/autorun.py 78.51% <0%> (-4.14%) ⬇️
scapy/contrib/homeplugav.py 90.52% <0%> (-1.95%) ⬇️
scapy/contrib/automotive/obd/mid/mids.py 98.37% <0%> (-1.63%) ⬇️
scapy/contrib/http2.py 95.48% <0%> (-1.32%) ⬇️
... and 6 more

@MaoJianwei
fix bug for issue #1862
b8604b1 
Improve the method to recognize tcpdump existence, covering more output cases in which tcpdump does not support '--version' option
Issue link:
#1862
@MaoJianwei
Copy link
Contributor Author

I have fixed this, please review again, thanks :) @guedou

p-l-
p-l- requested changes Sep 19, 2019
View reviewed changes
Copy link
Member

@p-l- p-l- left a comment

Choose a reason for hiding this comment

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

flake8 fails

scapy/arch/common.py Outdated
# On some systems, --version does not exist on tcpdump
return proc.returncode == 0 or output.startswith(b'Usage: tcpdump ')
return proc.returncode == 0 \
or output.startswith(b'Usage: tcpdump ') \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
or output.startswith(b'Usage: tcpdump ') \
or output.startswith(b'Usage: tcpdump ') \

scapy/arch/common.py Outdated
return proc.returncode == 0 or output.startswith(b'Usage: tcpdump ')
return proc.returncode == 0 \
or output.startswith(b'Usage: tcpdump ') \
or output.startswith(b'tcpdump: unrecognized option')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
or output.startswith(b'tcpdump: unrecognized option')
or output.startswith(b'tcpdump: unrecognized option')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I have fixed it, please review again, thanks :)

@MaoJianwei
fix bug for issue #1862
0086afd 
Improve the method to recognize tcpdump existence, covering more output cases in which tcpdump does not support '--version' option
Issue link:
#1862
@MaoJianwei
Copy link
Contributor Author

ok, I have fixed it, please review again, thanks :) @p-l-

@p-l- p-l- merged commit 265ee84 into secdev:master Sep 20, 2019
rafajot
rafajot reviewed Oct 2, 2019
View reviewed changes
Copy link

@rafajot rafajot left a comment

Choose a reason for hiding this comment

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

I think on my OS it's not going to work:

# cat /etc/redhat-release
CentOS release 6.10 (Final)
# tcpdump --version
tcpdump version 4.1-PRE-CVS_2017_03_21
libpcap version 1.4.0
Usage: tcpdump [-aAdDefhIJKlLnNOpqRStuUvxX] [ -B size ] [ -c count ]
                [ -C file_size ] [ -E algo:secret ] [ -F file ] [ -G seconds ]
                [ -i interface ] [ -j tstamptype ] [ -M secret ]
                [ -Q|-P in|out|inout ]
                [ -r file ] [ -s snaplen ] [ -T type ] [ -w file ]
                [ -W filecount ] [ -y datalinktype ] [ -z command ]
                [ -Z user ] [ expression ]
#

@KingsleyXie
Copy link

Got the same problem and similar output with rafajot on CentOS, tcpdump command returns 1 and the output does not meet with current check logic:

# cat /etc/redhat-release
CentOS Linux release 6.2 (Final)

# tcpdump --version
tcpdump version 4.1-PRE-CVS_2012_03_26
libpcap version 1.0.0
Usage: tcpdump [-aAdDefIKlLnNOpqRStuUvxX] [ -B size ] [ -c count ]
                [ -C file_size ] [ -E algo:secret ] [ -F file ] [ -G seconds ]
                [ -i interface ] [ -M secret ] [ -r file ]
                [ -s snaplen ] [ -T type ] [ -w file ] [ -W filecount ]
                [ -y datalinktype ] [ -z command ] [ -Z user ]
                [ expression ]

# echo $?
1

So one more condition or output.startswith(b'tcpdump version') \ should be added or maybe just consider changing the check logic, will 'Usage: tcpdump' in output be better for this?

@guedou guedou mentioned this pull request Oct 30, 2019
Merged
@guedou
Copy link
Member

guedou commented Oct 30, 2019

PR #2320 fixes the CentOS issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guedou guedou guedou left review comments

@p-l- p-l- p-l- approved these changes

+1 more reviewer

@rafajot rafajot rafajot left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tcpdump check error in centos
5 participants
@MaoJianwei @guedou @KingsleyXie @rafajot @p-l-