Skip to content

Commit 79bad07

Browse files
YHNdnzjbluca
authored andcommitted
core/executor: avoid double closing serialization fd
Before this commit, between fdopen() (in parse_argv()) and fdset_remove(), the serialization fd is owned by both arg_serialization FILE stream and fdset. Therefore, if something wrong happens between the two calls, or if --deserialize= is specified more than once, we end up closing the serialization fd twice. Normally this doesn't matter much, but I still think it's better to fix this. Let's call fdset_new_fill() after parsing serialization fd hence. We set the fd to CLOEXEC in parse_argv(), so it will be filtered when the fdset is created. While at it, also move fdset_new_fill() under the second log_open(), so that we always log to the log target specified in arguments.
1 parent 2292d37 commit 79bad07

File tree

1 file changed

+14
-20
lines changed

1 file changed

+14
-20
lines changed

src/core/executor.c

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -132,24 +132,22 @@ static int parse_argv(int argc, char *argv[]) {
132132
break;
133133

134134
case ARG_DESERIALIZE: {
135+
_cleanup_close_ int fd = -EBADF;
135136
FILE *f;
136-
int fd;
137137

138138
fd = parse_fd(optarg);
139139
if (fd < 0)
140-
return log_error_errno(
141-
fd,
142-
"Failed to parse serialization fd \"%s\": %m",
143-
optarg);
140+
return log_error_errno(fd,
141+
"Failed to parse serialization fd \"%s\": %m",
142+
optarg);
144143

145144
r = fd_cloexec(fd, /* cloexec= */ true);
146145
if (r < 0)
147-
return log_error_errno(
148-
r,
149-
"Failed to set serialization fd \"%s\" to close-on-exec: %m",
150-
optarg);
146+
return log_error_errno(r,
147+
"Failed to set serialization fd %d to close-on-exec: %m",
148+
fd);
151149

152-
f = fdopen(fd, "r");
150+
f = take_fdopen(&fd, "r");
153151
if (!f)
154152
return log_error_errno(errno, "Failed to open serialization fd %d: %m", fd);
155153

@@ -167,8 +165,7 @@ static int parse_argv(int argc, char *argv[]) {
167165
}
168166

169167
if (!arg_serialization)
170-
return log_error_errno(SYNTHETIC_ERRNO(EINVAL),
171-
"No serialization fd specified.");
168+
return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "No serialization fd specified.");
172169

173170
return 1 /* work to do */;
174171
}
@@ -199,10 +196,6 @@ int main(int argc, char *argv[]) {
199196
log_set_prohibit_ipc(true);
200197
log_setup();
201198

202-
r = fdset_new_fill(/* filter_cloexec= */ 0, &fdset);
203-
if (r < 0)
204-
return log_error_errno(r, "Failed to create fd set: %m");
205-
206199
r = parse_argv(argc, argv);
207200
if (r <= 0)
208201
return r;
@@ -211,17 +204,18 @@ int main(int argc, char *argv[]) {
211204
log_set_prohibit_ipc(false);
212205
log_open();
213206

207+
/* The serialization fd is set to CLOEXEC in parse_argv, so it's also filtered. */
208+
r = fdset_new_fill(/* filter_cloexec= */ 0, &fdset);
209+
if (r < 0)
210+
return log_error_errno(r, "Failed to create fd set: %m");
211+
214212
/* Initialize lazily. SMACK is just a few operations, but the SELinux is very slow as it requires
215213
* loading the entire database in memory, so we will do it lazily only if it is actually needed, to
216214
* avoid wasting 2ms-10ms for each sd-executor that gets spawned. */
217215
r = mac_init_lazy();
218216
if (r < 0)
219217
return log_error_errno(r, "Failed to initialize MAC layer: %m");
220218

221-
r = fdset_remove(fdset, fileno(arg_serialization));
222-
if (r < 0)
223-
return log_error_errno(r, "Failed to remove serialization fd from fd set: %m");
224-
225219
r = exec_deserialize_invocation(arg_serialization,
226220
fdset,
227221
&context,

0 commit comments

Comments
 (0)