Re: [OE-core] [pseudo][PATCH] pseudo_util.c: Open file with O_CLOEXEC to avoid fd leak
On Thu, 9 Apr 2020 06:30:37 + "Yu, Mingli" wrote: > We are trying to fix the issue as > http://bugzilla.yoctoproject.org/show_bug.cgi?id=13311 stated. Hmm. I think there's a more subtle logic bug here, and also one of the diagnostics there is a debug message I didn't notice when I made the commit, but it never comes up unless you run with PSEUDO_DEBUG_FILE. I'm gonna think about it more. I'm now about half convinced that the patch is actually fine as-is for clients, it's just on the server side that I think we'd see problems. -s -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#137155): https://lists.openembedded.org/g/openembedded-core/message/137155 Mute This Topic: https://lists.openembedded.org/mt/72889243/21656 Group Owner: openembedded-core+ow...@lists.openembedded.org Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [OE-core] [pseudo][PATCH] pseudo_util.c: Open file with O_CLOEXEC to avoid fd leak
Thanks Seebs's update! We are trying to fix the issue as http://bugzilla.yoctoproject.org/show_bug.cgi?id=13311 stated. Thanks, Mingli From: Seebs Sent: Thursday, April 9, 2020 13:25 To: Yu, Mingli Cc: openembedded-core@lists.openembedded.org ; MacLeod, Randy ; pave...@axis.com Subject: Re: [OE-core] [pseudo][PATCH] pseudo_util.c: Open file with O_CLOEXEC to avoid fd leak On Thu, 9 Apr 2020 10:38:41 +0800 "Yu, Mingli" wrote: > - fd = open(pseudo_path, O_WRONLY | O_APPEND | O_CREAT, 0644); > + fd = open(pseudo_path, O_WRONLY | O_APPEND | O_CREAT | > O_CLOEXEC, 0644); I'm not confident in this. This code is shared between the pseudo client and the pseudo server. In the pseudo server, it's possible that we coincidentally end up with fd being 2 already. In that case, we'd end up with this fd on 2, but then on exec (such as when execing the server) we'd possibly end up with it closed? Looking more closely, dup2 will clear the CLOEXEC flag, so in the server path, if prefer_fd is 2 and fd doesn't start out as 2, we end up with it cleared... But also so far as I can tell the code around the dup2 call there is probably generally wrong; it's explicitly closing the previous holder of that descriptor, which is unnecessary, and checking whether the file descriptor is already the desired one, which is also unnecessary. So this might need to be revisited. Looking at it more, the only time this seems like it should be relevant would be in the case where we're in the pseudo client, but PSEUDO_DEBUG_FILE is set in the environment. Otherwise, the client should end up just using stderr, and the server shouldn't care because it's expecting this to be fd 2 and we probably want the child process to end up with fd 2 open. What's the observed failure mode that we're fixing? -s -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#137135): https://lists.openembedded.org/g/openembedded-core/message/137135 Mute This Topic: https://lists.openembedded.org/mt/72889243/21656 Group Owner: openembedded-core+ow...@lists.openembedded.org Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [OE-core] [pseudo][PATCH] pseudo_util.c: Open file with O_CLOEXEC to avoid fd leak
On Thu, 9 Apr 2020 10:38:41 +0800 "Yu, Mingli" wrote: > - fd = open(pseudo_path, O_WRONLY | O_APPEND | O_CREAT, 0644); > + fd = open(pseudo_path, O_WRONLY | O_APPEND | O_CREAT | > O_CLOEXEC, 0644); I'm not confident in this. This code is shared between the pseudo client and the pseudo server. In the pseudo server, it's possible that we coincidentally end up with fd being 2 already. In that case, we'd end up with this fd on 2, but then on exec (such as when execing the server) we'd possibly end up with it closed? Looking more closely, dup2 will clear the CLOEXEC flag, so in the server path, if prefer_fd is 2 and fd doesn't start out as 2, we end up with it cleared... But also so far as I can tell the code around the dup2 call there is probably generally wrong; it's explicitly closing the previous holder of that descriptor, which is unnecessary, and checking whether the file descriptor is already the desired one, which is also unnecessary. So this might need to be revisited. Looking at it more, the only time this seems like it should be relevant would be in the case where we're in the pseudo client, but PSEUDO_DEBUG_FILE is set in the environment. Otherwise, the client should end up just using stderr, and the server shouldn't care because it's expecting this to be fd 2 and we probably want the child process to end up with fd 2 open. What's the observed failure mode that we're fixing? -s -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#137132): https://lists.openembedded.org/g/openembedded-core/message/137132 Mute This Topic: https://lists.openembedded.org/mt/72889243/21656 Group Owner: openembedded-core+ow...@lists.openembedded.org Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[OE-core] [pseudo][PATCH] pseudo_util.c: Open file with O_CLOEXEC to avoid fd leak
From: Pavel Modilaynen Use close-on-exec (O_CLOEXEC) flag when open log file to make sure its file descriptor is not leaked to parent process on fork/exec. Signed-off-by: Mingli Yu --- pseudo_util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pseudo_util.c b/pseudo_util.c index c867ed6..0ec527b 100644 --- a/pseudo_util.c +++ b/pseudo_util.c @@ -1594,7 +1594,7 @@ pseudo_logfile(char *filename, char *defname, int prefer_fd) { } free(filename); } - fd = open(pseudo_path, O_WRONLY | O_APPEND | O_CREAT, 0644); + fd = open(pseudo_path, O_WRONLY | O_APPEND | O_CREAT | O_CLOEXEC, 0644); if (fd == -1) { pseudo_diag("help: can't open log file %s: %s\n", pseudo_path, strerror(errno)); } else { -- 2.7.4 -=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#137129): https://lists.openembedded.org/g/openembedded-core/message/137129 Mute This Topic: https://lists.openembedded.org/mt/72889243/21656 Group Owner: openembedded-core+ow...@lists.openembedded.org Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-