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

[Christian Seiler]

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?

Regards,
Christian