4:03 p.m.
* src/util/util.c (__virExec): Don't use FD_ISSET on out-of-bounds fd.
---
Noticed this one by inspection, while investigating
https://bugzilla.redhat.com/show_bug.cgi?id=659855
Don't know if it's the root cause of the crash in that bug, though.
src/util/util.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 79ca5d3..1b5bc68 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -570,8 +570,7 @@ __virExec(const char *const*argv,
i != null &&
i != childout &&
i != childerr &&
- (!keepfd ||
- !FD_ISSET(i, keepfd))) {
+ (!keepfd || (i < FD_SETSIZE && !FD_ISSET(i, keepfd)))) {
tmpfd = i;
VIR_FORCE_CLOSE(tmpfd);
}
--
1.7.3.2
1:36 p.m.
On 12/03/2010 05:03 PM, Eric Blake wrote:
* src/util/util.c (__virExec): Don't use FD_ISSET on
out-of-bounds fd.
---
Noticed this one by inspection, while investigating
https://bugzilla.redhat.com/show_bug.cgi?id=659855
Don't know if it's the root cause of the crash in that bug, though.
src/util/util.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 79ca5d3..1b5bc68 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -570,8 +570,7 @@ __virExec(const char *const*argv,
i != null&&
i != childout&&
i != childerr&&
- (!keepfd ||
- !FD_ISSET(i, keepfd))) {
+ (!keepfd || (i< FD_SETSIZE&& !FD_ISSET(i, keepfd)))) {
tmpfd = i;
VIR_FORCE_CLOSE(tmpfd);
}
ACK. Definitely this could be bad news if OPEN_MAX > FD_SETSIZE, and
even if that's not possible, it doesn't hurt to check anyway.
3:22 p.m.
On 12/10/2010 12:36 PM, Laine Stump wrote:
On 12/03/2010 05:03 PM, Eric Blake wrote:
> * src/util/util.c (__virExec): Don't use FD_ISSET on out-of-bounds fd.
> ---
>
> Noticed this one by inspection, while investigating
> https://bugzilla.redhat.com/show_bug.cgi?id=659855
>
> Don't know if it's the root cause of the crash in that bug, though.
Actually, on re-reading, I think that bug report was not an actual
crash, so much as someone trying to understand the original code and
providing a gdb backtrace of a working setup based on a misreading of
the code base.
> - (!keepfd ||
> - !FD_ISSET(i, keepfd))) {
> + (!keepfd || (i< FD_SETSIZE&& !FD_ISSET(i, keepfd)))) {
> tmpfd = i;
> VIR_FORCE_CLOSE(tmpfd);
> }
ACK. Definitely this could be bad news if OPEN_MAX > FD_SETSIZE, and
even if that's not possible, it doesn't hurt to check anyway.
On Linux, FD_SETSIZE is 1024, and getconf(_SC_OPEN_MAX) starts at 1024
(I don't know if it can dynamically grow larger, but if we have an fd
that large, we probably have bigger problems of an fd leak on our hands
before this bug would have bitten us); but unless getconf can return a
dynamic value for _SC_OPEN_MAX, the undefined behavior would never occur.
On the other hand, cygwin is a platform where FD_SETSIZE is 64, but
getconf(_SC_OPEN_MAX) is dynamic, starting at 32, but growing as large
as 3200. Thus, even having as low as fd 65 open, and having the stack
overflow of reading beyond keepfd accidentally see that fd as one to be
kept, can leak fd 65 into the child (and only depending on what was at
the out-of-bounds memory). Still a bit high to have that many fds open
at once, but much more feasible than the Linux limit, and definitely a
platform where OPEN_MAX can be larger than FD_SETSIZE.
I've pushed this now.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5127
days inactive
5134
days old
2 comments
2 participants
participants (2)
-
Eric Blake -
Laine Stump