Aren/haiku
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

kernel: Rework the FD disconnect feature (again).

Browse Source 
axeld's solution from 2015 worked in that it solved the panics and
problems with leaking FDs ... but only if nobody actually tried to
use the FDs again. As you can see in the diff of the previous commit,
in allowing closed FDs (which have NULL "ops") to be returned by get_fd,
all consumers of the get_fd API (so, pretty much most functions in
vfs.cpp and fd.cpp) have to check *both* that (1) the fd is not NULL,
and (2) the fd does not have O_DISCONNECT set.

Besides missing a large majority of consumers of get_fd (which caused
ticket #14532 and also the first half of ticket #14756, probably among
others, as I haven't reviewed all NULL-dereference-in-VFS tickets yet)
this solution missed the fact that calling get_fd increments the reference
count, but then exiting the exact same way as if the FD was NULL
(without putting it) when it is disconnected *also* leaks the FD.

As it turns out, a not insignificant number of applications try
to do this, which (depending on whether you went through one of the
'lucky' functions axeld's commit touched) either (1) leaked the FD,
or (2) caused a kernel panic.

Now, we could go through and add O_DISCONNECT checks to every single
consumer of get_fd followed by put_fd to get the proper behavior ...
but that would be the same thing as just returning NULL here and not
incrementing the reference count.

So it seems the first part of axeld's solution (don't set open_count
or ref_count to -1 but leave them as-is) is the only change necessary.

A few places where there were legitimately missing O_DISCONNECT checks
(some originally added by axeld) are (re-)added in the next commit.
Otherwise this seems to be the more robust solution. (But I wonder why
nobody caught this in the code review axeld asked for in the commit
and the ticket back in 2015? Did nobody notice the unbalanced get/put?)

Fixes #14532, part of #14756, and probably any other NULL dereferences
in VFS I/O functions (XHCI is especially good at exposing these)
that are lingering around on the bugtracker.
This commit is contained in:
Augustin Cavalier 2018-12-12 18:42:03 -05:00
parent 15ec0b5cc8
commit 64d1636fea
1 changed files with 6 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
src/system/kernel/fs/fd.cpp
View File

@ -224,13 +224,11 @@ put_fd(struct file_descriptor* descriptor)
descriptor->ops->fd_free(descriptor);
// prevent this descriptor from being closed/freed again
descriptor->open_count = -1;
descriptor->ref_count = -1;
descriptor->ops = NULL;
descriptor->u.vnode = NULL;
// the file descriptor is kept intact, so that it's not
// reused until someone explicetly closes it
// reused until someone explicitly closes it
}
}
@ -299,13 +297,12 @@ get_fd_locked(struct io_context* context, int fd)
struct file_descriptor* descriptor = context->fds[fd];
if (descriptor != NULL) {
// Disconnected descriptors cannot be accessed anymore
// disconnected descriptors cannot be accessed anymore
if (descriptor->open_mode & O_DISCONNECTED)
descriptor = NULL;
else {
TFD(GetFD(context, fd, descriptor));
inc_fd_ref_count(descriptor);
}
return NULL;
TFD(GetFD(context, fd, descriptor));
inc_fd_ref_count(descriptor);
}
return descriptor;