[lxc-devel] [PATCH 1/2] fix trivial off by one error

[Dwight Engen]

On Tue, 18 Sep 2012 22:18:03 +0200
Christian Seiler <christian at iwakd.de> wrote:

> Hi,
> 
> Just a heads up:
> 
> > Since the if uses >=, the - 1 is not needed and the MAXFDS'th
> > entry in the fds array can be used.
> 
> This was from part of one of my patches regarding lxc-attach and it is
> NOT an off-by-one error, it is meant to be this way. The problem is
> that the array has to be traversed later (both for completing the
> attach operation or aborting it), see
> lxc_cgroup_(dispose|finish)_attach:
> 
> | for (; *fds >= 0; fds++) {
> 
> This loop will result in a potential buffer overflow with your change.
> 
> Obviously, you could rewrite it to also check against the offset of
> the beginning of the array and compare that to MAXFDS in those loops,
> but that makes the traversal loop more complicated to read and
> whether the maximum number of cgroup controllers mounted that
> lxc-attach supports is 255 or 256 shouldn't matter much. On the other
> hand, this means the overflow wouldn't occur in practice - however,
> if at some later point somebody should change MAXFDS to 16 to save
> some memory, for example on embedded systems, then this could lead to
> undefined behaviour - and even potential security wholes if
> lxc-attach is installed as setuid root.
> 
> But because this appears to be something that needs clarification,
> perhaps you could change your patch to just add a comment explaining
> the situation?

Ahh, yes I see. I think the name MAXFDS threw me off, its really a -1
terminated array with MAXFDS - 1 valid slots.

Do you think mallocing an fd_set and using FD_SET() and friends
would be better? The (dispose|finish) loops would visit FD_SETSIZE bits
with an FD_ISSET() test, which is more work than you have currently
with the early out, but we would probably save on the initialization
with FD_ZERO(). I don't know if lxc_cgroup_(dispose|finish)_attach is
performance critical.

Or I can just add a comment :)