Set FD_CLOEXEC on the LMDB FD manually #287
Conversation
| } | ||
| if (db_fd_flags != -1) { | ||
| fcntl (db_fd, F_SETFD, db_fd_flags | FD_CLOEXEC); | ||
| } |
There was a problem hiding this comment.
Could you remove the extra brackets? If there's just a single line following an if statement, they should be omitted in AppStream's coding style
src/as-cache.c
Outdated
| https://www.openldap.org/lists/openldap-bugs/201702/msg00003.html */ | ||
| rc = mdb_env_get_fd (priv->db_env, &db_fd); | ||
| if (rc == MDB_SUCCESS) { | ||
| db_fd_flags = fcntl (db_fd, F_GETFD); |
There was a problem hiding this comment.
Could you move the db_fd_flags variable into this block, so it only is visible when we actually need it, and also move the if (db_fd_flags != -1) into this block as well, to logically group this together?
There was a problem hiding this comment.
It looked to me like it was using the old C style to declare all variables at the function start...
Done.
There was a problem hiding this comment.
Yeah, some older parts of the code do, but for new code we can use fancy "new" things! - Even the cleanup attribute and g_autofree, which is one of the best things that ever happened to a C developer ;-)
|
Oof, I kind of implicitly expected LMDB to do the right thing here and already use |
Currently the FD referring to the appstreacm-cache-FOO.mdb is leaked into child processes. The only way to fix is in a race-free way is by passing O_CLOEXEC when opening it inside LMDB, but that's currently not done.
|
Looks good to me, thank you for the quick changes :-) |
|
Thanks for the quick merge! |
Currently the FD referring to the appstreacm-cache-FOO.mdb is leaked into
child processes. The only way to fix is in a race-free way is by passing
O_CLOEXEC when opening it inside LMDB, but that's currently not done.