use cloexec in more places #44
Conversation
The current logic will leak open handles into programs we exec. Change these to use CLOEXEC to avoid that leakage since leaving the handles open isn't really useful.
| @@ -80,7 +80,7 @@ static void | |||
| file_open(stdlog_channel_t ch) | |||
| { | |||
| if (ch->d.file.fd == -1) { | |||
| if((ch->d.file.fd = open(ch->d.file.name, O_WRONLY|O_CREAT|O_APPEND, 0660)) < 0) | |||
| if((ch->d.file.fd = open(ch->d.file.name, O_WRONLY|O_CREAT|O_APPEND|O_CLOEXEC, 0660)) < 0) | |||
There was a problem hiding this comment.
Prior to this change I could fork off a child worker process (or multiple processes) and it/they could continue to log with this channel. After this change, the child(ren) will not be able to log and log() calls will simply return -1. Is it thought that the caller of stdlog_log() should implement retry logic that includes re-opening the channel with -1 is returned? The man page doesn't mention that. If that isn't (current) expected (or practiced) behavior, I think this is a breaking change.
There was a problem hiding this comment.
CLOEXEC affects fd handling across execs, not across forks. so if you have an app using liblogging and it forks and does its own processing, all of that continues to work. what doesn't work is if you tried to exec a program and that program tried to write to the fd directly (because the handle would be closed automatically by the kernel). although that design pattern is kind of broken in the first place as how would that program know what fd to even write to in the first place ?
There was a problem hiding this comment.
Of course it does! Silly me, what was I thinking? Kindly proceed to commit!
| @@ -28,6 +28,14 @@ | |||
| #include <sys/un.h> | |||
| #include "stdlog.h" | |||
|
|
|||
| #ifndef O_CLOEXEC | |||
There was a problem hiding this comment.
i didn't really see a better place to stuff these fallback defines, but i'm not super familiar with the codebase ...
There was a problem hiding this comment.
Is the code used on any system that does not define O_CLOEXEC?
There was a problem hiding this comment.
Actually, if these flags are not defined then I think rather than defining them to be no-op flags it would be better to call fcntl() to set the close-on-exec flag as a more complete fallback solution.
There was a problem hiding this comment.
i don't know about usage on other platforms, but it's also a common fallback for older Linux systems that don't have it
yes, adding fallback to fcntl would be a more complete solution, but i figured it wasn't worth the effort. if someone wants to, they could make that change easily on top of my change here, so we at least get incremental improvements.
| #ifndef SOCK_CLOEXEC | ||
| #define SOCK_CLOEXEC 0 | ||
| #endif | ||
|
|
||
| #define __STDLOG_MSGBUF_SIZE 4096 | ||
| #ifndef STDLOG_INTERN_H_INCLUDED |
There was a problem hiding this comment.
I know this isn't a part of your change, but I find it odd that the include guard is not at the top of the file protecting all the includes and definitions in this file. Can anyone say why it is done this way?
| #define O_CLOEXEC 0 | ||
| #endif | ||
|
|
||
| #ifndef SOCK_CLOEXEC |
There was a problem hiding this comment.
Same question here as for O_CLOEXEC, although SOCK_CLOEXEC seems to be newer and is probably less likely to be defined than O_CLOEXEC.
In any case, assuming that these fallback defines are needed, I can't think of a better place for them offhand.
No description provided.