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

This fixes a number of problems around the pcap-fifo handling #8

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pjsg
Copy link

@pjsg pjsg commented Sep 29, 2022

The underlying issue is that writing to a fifo typically does not write the full amount of data. The code in out_flush tries to handle this, but gets it wrong. In particular, the existing code tends to go into a stack overflow after writing the first block of data to the fifo. This causes a memory overwrite, and then the file number used for subsequent writes is invalid, and so it returns EBADF. The is caught, adn the code then tries to shut down cleanly -- which calls out_close which calls out_flush which fails to write (with EBADF) and no we are doomed.

Also, the out_close function assumed that out_flush would always flush all the buffered data -- and it doesn't.

This PR fixes those issues.

@cmikk
Copy link
Contributor

cmikk commented Oct 5, 2022

Thanks for investigating this issue and sharing your patches.

We will be pushing fixes to the size_t underflow error and out_flush/stop recursion shortly, and doing a further review of this code for more potential fixes.

Regarding the out_close leaving buffered data behind, this is by design: discarding any buffered data which cannot be immediately written reduces the time to re-establish the connection when restarting sratool, in turn reducing the overall amount of data lost or missed. The "flush all the buffered data" functionality will not be included in the upcoming change set, but we are open to making it configurable if this behavior is still desired.

@pjsg
Copy link
Author

pjsg commented Oct 5, 2022

My use case is running in a kubernetes environment. When a new version of the container gets deployed, I want a graceful shutdown of the old container. I guess I can live with the not flushing of the output.

My actual script is:

    mkdir -p /pvc/ch14
    mkfifo /tmp/pcap
    START=`date +%Y%m%dT%H%M%S`
    (tcpdump -r /tmp/pcap -G 900 -w /pvc/ch14/%Y%m%dT%H%M%S_${START}.ch14.pcap -z /compress; /compress `ls -t /pvc/ch14/*${START}.ch14.pcap | head -n 1` ) &
    trap 'pkill -TERM sratunnel' TERM
    sratunnel -c 14 -s ssh:[email protected] -o pcap-fifo:/tmp/pcap -w ch=14 -n ~/axa_config &
    wait
    wait

This uses tcpdump to split the pcap stream into 15 minute chunks that are written to individually named files and then compressed. The container SIG_TERM is forwarded to sratunnel which shuts down, and the tcpdump drains the fifo and it too exits. Unfortunately, it doesn't call the -z command on termination, so that has to be done specially. Finally the script finishes and the container exits.

reedjc added a commit that referenced this pull request Apr 10, 2023
Squashed commit of the following:

1930d86 miscellaneous updates to README and manpages for release 3.0.1
18552be Merge branch 'release-3.0.0'
9d4ee25 Pull request #29: Release 3.0.0 lintian nits
c0bf1f2 Add debian/copyright.
6e4bc30 Lintian nits.
860ac0e update copyrights
7460d28 remove reference to -c option since this doc is included in manpages without it
70ae93a Merge branch 'next' into release-3.0.0
73b0293 Pull request #27: Eliminate crash condition caused when an insecure+apikey connection erroneously attempts to invoke the openssl layer.
ada5e5f Eliminate crash condition caused when an insecure+apikey connection erroneously attempts to invoke the openssl layer.
b52881f add two symbols and sort
ab5d87d various updates for package name change to libaxa3
b665462 change libtool versioning for this major release, changes to libaxa3 debian package
d4898af update changelog for 3.0.0-1
1d4c42c Pull request #26: Fix pcap file next
b783e76 sratunnel: catch stop() recursion on flush error
f89e88e sratunnel: Fix pcap out buffer space calculation
e32ecf9 remove sentence about (shared) feature from sratunnel manual
16da5ab Pull request #23: Remove more custom code
0227efc document -I insecure option
038ea06 remove -i and -S from tunnels usage output
89a1546 Excise all references to lmdb-related functionality.
e9d82c9 Add insecure mode command line option to sratunnel utility. Update usage/man page documentation to include mentions of insecure mode.
47330ed Added back support for verbose printing of nmsg payloads in sratool.
0c9e31c More cleanup of dead code/improperly scoped functions.
eae6576 Remove more now-deprecated code relating to printing axa nmsg payloads. Restore basic watch hit information to sratool display.
fd25674 Pull request #18: Feature/RELENG-480 release axa
7173f5e Pull request #20: Remove custom code
a4f9150 Correct mismatched external declaration.
38896f5 Merge branch 'master' into next
2e6402a Merge branch 'feature/RELENG-480-release-axa' of ssh://bitbucket.it.fsi.io:7999/axa/axa-tools into RELENG-480-release-axa
735001b Remove time formatting code that can lead to a crash but the result of which is never even used.
471ec8d Remove various extraneous dependencies.
2280f69 Fix minor unit test failure caused by removal of axa_domain_to_str().
fc26d00 Replace invocation of axa_wdns_res() with wdns_res_to_str().
d5d0ecf change package version to 3.0.0
f809a0d Replace invocations of axa_domain_to_str() with wdns_domain_to_str().
877589b Replace definition dependencies on nameser.h with wdns.
91024d8 Use wdns_rdata_to_str() and nmsg_message_to_json() where possible, instead of relying on custom routines.
f1dd9b2 Minor man page adjustment.
8f64d4d update test for improved error message
eea45c6 Include more explicit explanation of error watches.
9afa03a Fix minor man page variance.
24028dd Changes to allow for insecure apikey mode.
3a1b3b8 Removed remnant -S references in application usage display.
60f0f8f Got rid of all references to deprecated client-side -S <certs_dir> option.
3ad7e5d Fix slightly incorrect documentation of client "CONNECT" command.
3cdf77b Clarify usage of tcp: and unix: connection paths in shared man page.
647f174 Clarify all documentation of rate limit(s).
ceb25e9 Switch from local to GMT time in missed packet display.
773e3ce Fix incorrect display of "buffering" command. Use more specific description "forwarding mode" in place of "output mode".
184b720 Friendlier error message for valid commands with unrecognized subcommands.
bc5602b Disambiguate the use of the mandatory leading tag parameter in various cli commands.
c91e449 Merge branch 'master' into next
c06ec21 Pull request #16: DEV-1497 move ssl termination from axa codebase to nginx
828917c DEV-1497 move ssl termination from axa codebase to nginx
cffb1ec Bump version to 3.0 in configure.ac
cadcdec Fix bug in changelog package name
6b97504 DEV-1516 remove use of sentry term in rad modules
066c953 DEV-1516 remove use of sentry term in rad modules
b8da838 DEV-1334 sratunnel will not connect from debian10 buster or newer ssl implementations next
27c0d07 Merge branch 'next' into DEV-1334-sratunnel-will-not-connect-from-debian10-buster-or-newer-ssl-implementations_next
325ed29 DEV-1345 axa should drop tls and ssh auth methods
ba0dff1 Revert default cipher_list back to ECDHE-RSA-AES256-GCM-SHA384
2c01db7 Pull request #8: DEV-1288 axa add tests for new code
330b1d7 Fix typo and copyright line in tests/test-tools-with-server.sh.in
554c0fb Merge remote-tracking branch 'origin/next' into DEV-1288-axa-add-tests-for-new-code
a5eba42 DEV-1345 axa should drop tls and ssh auth methods Bump versions to 3.0.0 Remove ssh tunnel handling (axaproxy)
03c31b7 Change bits argument to DSA_generate_parameters_ex() from 1024 to 2048
5ebb249 s/        /\t/g
c44c354 tunneling tools: "-I" needs "-i"
1d23235 sratunnel tests
6925bf4 New test scripts for tests WITH a running server
49375b1 Merge pull request #7 in AXA/axa-tools from remove-brand-refs to next
2a6da3a removing Brand and Domain Sentry references
abd723c Merge pull request #6 in AXA/axa-tools from DEV-1288-axa-add-tests-for-new-code to next
b686160 Fix unused variables in test_kickfile_rotate()
0379b3e remove errant repeated line from Makefile
4a73655 New unit test for kickfile functions
c665fb7 Merge pull request #5 in AXA/axa-tools from DEV-1287-axa-switch-enum-errors to next
2211595 Fix white spacing in test-stats.c
e20871a Fix memory alignment issue around use of ch_mask.m
7c6b363 Add missing enums to switch statements in empty_test() and truncated_test()
165426e Fix unaligned pointer argument to axa_get_bitwords()
fb84238 Merge pull request #2 in AXA/axa-tools from libprotobuf-dependency-fix to next
ca7aa5e Move libprotobuf-c0-dev to libprotobuf-c-dev .
71170ce Merge pull request #1 in AXA/axa-tools from add_sratunnel_max_filesize_opt to next
0d9aa75 {rad,sra}tunnel: check return value of lmdb_init() and exit on failure
afb9ab7 non-demonstrative assorted cleanup
c91062e {rad,sra}tunnel: properly handle corner case when lmdb runs out of memory
170d718 remove debug print
934f25a {rad,sra}tunnel: fix tsindex so an index is always written for the first nmsg in a file (offset 0)
ca211e5 {rad,sra}tunnel: fix file rotation lockstepping
e25f425 use the prefer AXA macro for gcc builtin
e2e67a8 {rad,sra}tunnel: keep rotated lmdb files in lockstep with the nmsg data files they back
9a2d4d8 give kickfile filenames the option to use a hint
aa6d90b {rad,sra}tunnel: remove final lmdb lockfile (for -i + -k)
bc34180 {rad,sra}tunnel: update documentation to better explain -i
de6c427 {rad,sra}tunnel: upon exit, remove lmdb lockfile
84b2936 {rad,sra{tunnel: bind lmdb put/commit together
e60398b {sra,rad}tunnel: allow -k with -i initial commit
aceb19d errata: fix test
1511ff2 sratunnel: update copyright year for help blurb
c77c31e sratunnel: add documention for "-Z" option
2e5fec2 sratunnel: add "-Z size" option to clamp an output file to "size" bytes
30f5d5d Merge branch 'next' of ssh://bitbucket.it.fsi.io:7999/axa/axa-tools into next
a61cbbb Merge repo.fsi.io:axa/axa-tools into next
892e745 Merge branch 'master' into next
7375ae1  Merge branch 'fixgetopt' into 'next'
da88e0a add I: to getopt
729613e  Merge branch 'lmdb-sizeable' into 'next'
a9cf466 update help output
9bda9eb index size doc
7f7bf7e allow mdb mapsize as a param
cf2fa99 wrong error message
032dee5  Merge branch 'conf--optional' into 'next'
1d36017 make sure same config-file-test logic that is used in sratool/main.c also appears in sratunnel/main.c
3edfeec  Merge branch 'update_readme' into 'next'
aeffb13  Merge branch 'type_fix' into 'next'
25cb145 update google protobuf install instructions
81913c9 multiply by 1.0 to have the result be a double/float since casting causes -Wbad-function-cast to complain
d563dc1 update README
d2604e8  Merge branch 'more_small_fixes' into 'next'
f8275f2 small fix for small ppl
ee5bc5d clean up compiler warnings
00f31fd  Merge branch 'fix_issue112' into 'next'
30ebc81  Merge branch 'fix_issue111' into 'next'
5330459 update axa config tests
75b9f4f update axa config documentation
e52af07 allow axa config file to be optional
1493b25 {rad,sra}tunnel: fix bug in tsindex+append mode
48efbeb  Merge branch 'fix_clan_warnings' into 'next'
b24219e fix benign clang warnings
d86121d  Merge branch 'add_kickfile' into 'next'
756223e add debug message to inform user when connection to server is successful
e9d358c man page pedantics
746cc61 {rad,sra}tunnel: add kickfile option
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.

2 participants