Conversation
In order to satisfy static checkers that we don't have an fd leak, a9c61ff added a close() of c->pidfile_fd, amongst others. However, it only close()s it in the case where we daemonize into the background. While less universally useful in the foreground / non-daemon case, it's perfectly reasonable to still have a pidfile. We'll still write it, and we should still close it. Fixes: a9c61ff ("util, passt: Close daemon-lifetime fds on exit to avoid Coverity warning") Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Most functions in util.c are, well, utilities, that are useful in a bunch of places. close_open_files(), however, is very specific, it's only called from isolate_initial(), and performs a very specific step of our self isolation. So, it makes more sense as a function in isolate.c - move it there and rename it to isolate_fds(). In addition, call it directly from main() rather than from isolate_initial(). That's pretty arbitrary now, but will make some subsequent changes easier. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We parse --fd twice: once in isolate_initial() just to avoid clobbering the passed in fd. Then we parse it "for real" in conf(), to set c->fd_tap and other configuration variables. Change this, so that we return the value parsed early from isolate_initial() and set c->fd_tap from that. This doesn't accomplish much immediately, but will make some further cleanups possible. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We have two cases where we need to parse specific options early: conf_tap_fd() and conf_mode(). conf_tap_fd() has a slightly odd interface, requiring the caller to use getopt_long() to find the right option, then pass it in. Alter it to work like conf_mode() instead, where all the command line parsing logic is contained within the conf.c function. This is slightly more lines, but has a clearer division of responsibility. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Some users of passt pass an fd for the tap interface in, with the --fd parameter, rather than having passt open it itself. This requires some slightly fiddly logic in isolate_fds() so we don't close() it along with any other file descriptors leaked into us by the parent. More importantly, this is broken if the passed fd is 0, 1 or 2, since in that case we will assume it's a standard stream and close it in __daemon(). We explicitly disallow 1 or 2 in conf_tap_fd(), but 0 has been permitted since aa1cc89 ("conf: allow --fd 0"). It looks like the use case of the contributor of that patch didn't involve daemonizing passt. To fix this more robustly, use dup2() to move to the passed fd to 3. This removes the possibility of mixing it up with a standard stream, and as a bonus makes the close_range() logic much simpler. With isolate_fds() made safe for --fd 1 and --fd 2, we can remove the logic excluding those from conf_fd_tap(). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Usually fds 0-2 are stdin, stdout and stderr. However, there are certain use cases where passt can be invoked with one or more of those standard fds closed. In those cases, anything we open might be placed in one of the standard slots. For the handful of things we open early enough, this can be a problem because we close fds 0-2 in __daemon(), replacing them with dupes of /dev/null. We could avoid closing those fds in __daemon() if they're not standard streams. However, leaving things other than the standard streams in fds 0-2 is a footgun: a stray printf() that occurs in a circumstance it shouldn't could send harmful garbage to a device or socket. It's also likely to be confusing if debugging with strace or similar. To avoid this, fill any missing standard streams with a dupe of /dev/null, right after isolate_fds(). Since open()ing /dev/null itself could land in one of those fd 0-2 slots, we need to be careful when we close it not to leave a new gap. Link: https://bugs.passt.top/show_bug.cgi?id=215 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )