Skip to content

Commit b21a70c

Browse files
committed
mod_mimeparse: Don't close file descriptor already closed by GMime.
New warnings about closing file descriptors that were already closed alerted to a file descriptor we were trying to close after the library had already closed it for us. Since that isn't captured in the array in fd.c, add an API to indicate a file descriptor has already been closed outside of the BBS (e.g. in a library), so we can keep our state up to date while avoiding double closes.
1 parent 9bf4a36 commit b21a70c

File tree

3 files changed

+33
-5
lines changed

3 files changed

+33
-5
lines changed

bbs/fd.c

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -398,16 +398,17 @@ int __bbs_close(int fd, const char *file, int line, const char *func)
398398

399399
/* Detect attempts to close file descriptors we shouldn't be closing
400400
* (e.g. can happen if a file descriptor variable is initialized to 0 instead of -1) */
401-
if (fd <= 2 && (fd < 0 || strcmp(file, "system.c"))) { /* It's legitimate to close file descriptors 0, 1, and 2 when calling exec */
401+
if (unlikely(fd <= 2) && (fd < 0 || strcmp(file, "system.c"))) { /* It's legitimate to close file descriptors 0, 1, and 2 when calling exec */
402402
bbs_warning("Attempting to close file descriptor %d at %s:%d (%s)\n", fd, file, line, func);
403403
bbs_log_backtrace(); /* Get a backtrace to see what made the invalid close, in case immediate caller isn't enough detail. */
404404
}
405405
res = close(fd);
406406
if (res) {
407407
if (errno == EBADF && ARRAY_IN_BOUNDS(fd, fdleaks)) {
408-
__bbs_log(LOG_WARNING, 0, file, line, func, "Failed to close file descriptor %d: %s (previously closed at %s:%d)\n", fd, strerror(errno), fdleaks[fd].file, fdleaks[fd].line);
408+
__bbs_log(LOG_WARNING, 0, file, line, func, "Failed to close fd %d: %s (previously %s at %s:%d)\n",
409+
fd, strerror(errno), fdleaks[fd].isopen ? "opened" : "closed", fdleaks[fd].file, fdleaks[fd].line);
409410
} else {
410-
__bbs_log(LOG_WARNING, 0, file, line, func, "Failed to close file descriptor %d: %s\n", fd, strerror(errno));
411+
__bbs_log(LOG_WARNING, 0, file, line, func, "Failed to close fd %d: %s\n", fd, strerror(errno));
411412
}
412413
} else if (ARRAY_IN_BOUNDS(fd, fdleaks)) { /* && !res (implicit) */
413414
fdleaks[fd].isopen = 0;
@@ -417,7 +418,27 @@ int __bbs_close(int fd, const char *file, int line, const char *func)
417418
}
418419
FD_LOGF("%lu: %s:%d (%s) close(%d)\n", time(NULL), file, line, func, fd);
419420
return res;
420-
}
421+
}
422+
423+
/* gcc thinks func is unused in this function? */
424+
#pragma GCC diagnostic push
425+
#pragma GCC diagnostic ignored "-Wunused-parameter"
426+
int __bbs_mark_closed(int fd, const char *file, int line, const char *func)
427+
{
428+
/* Don't attempt to close the file descriptor, if it's already closed.
429+
* Something else may reuse it in the meantime and we could close something else.
430+
* Of course, if that's the case, we'll mess up our state array below,
431+
* but that's lower stakes than actually messing up program behavior. */
432+
if (ARRAY_IN_BOUNDS(fd, fdleaks)) {
433+
fdleaks[fd].isopen = 0;
434+
/* Update to where it was closed so we can debug attempts to close previously closed fds */
435+
COPY(fdleaks[fd].file, file);
436+
fdleaks[fd].line = line;
437+
}
438+
FD_LOGF("%lu: %s:%d (%s) mark_closed(%d)\n", time(NULL), file, line, func, fd);
439+
return 0;
440+
}
441+
#pragma GCC diagnostic pop
421442

422443
FILE *__bbs_fopen(const char *path, const char *mode, const char *file, int line, const char *func)
423444
{

include/bbs.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,13 +132,20 @@
132132
#define dup(a) __bbs_dup(a, __FILE__,__LINE__,__func__)
133133
#define eventfd(a,b) __bbs_eventfd(a, b, __FILE__,__LINE__,__func__)
134134

135+
/*! \brief Mark a file descriptor as closed. This is useful if
136+
* a file descriptor was opened by the BBS and closed by another library,
137+
* so we can keep the internal state table of file descriptors synchronized.
138+
* This function takes you at your word, so be sure it really is closed! */
139+
#define bbs_mark_closed(f) __bbs_mark_closed(f, __FILE__,__LINE__,__func__)
140+
135141
int __bbs_open(const char *file, int line, const char *func, const char *path, int flags, ...);
136142
int __bbs_pipe(int *fds, const char *file, int line, const char *func);
137143
int __bbs_socketpair(int domain, int type, int protocol, int sv[2], const char *file, int line, const char *func);
138144
int __bbs_socket(int domain, int type, int protocol, const char *file, int line, const char *func);
139145
int __bbs_accept(int socket, struct sockaddr *address, socklen_t *address_len, const char *file, int line, const char *func);
140146
int __bbs_eventfd(unsigned int initval, int flags, const char *file, int line, const char *func);
141147
int __bbs_close(int fd, const char *file, int line, const char *func);
148+
int __bbs_mark_closed(int fd, const char *file, int line, const char *func);
142149
FILE *__bbs_fopen(const char *path, const char *mode, const char *file, int line, const char *func);
143150
int __bbs_fclose(FILE *ptr);
144151
int __bbs_dup2(int oldfd, int newfd, const char *file, int line, const char *func);

modules/mod_mimeparse.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,7 @@ static GMimeMessage *mk_mime(const char *file)
366366
message = g_mime_parser_construct_message(parser, NULL);
367367

368368
g_object_unref(parser);
369-
close(fd);
369+
bbs_mark_closed(fd); /* g_object_unref closed our file descriptor for us */
370370

371371
if (!message) {
372372
bbs_error("Failed to parse message %s as MIME\n", file);

0 commit comments

Comments
 (0)