[lxc-devel] [PATCH 1/2] fix trivial off by one error
Since the if uses >=, the - 1 is not needed and the MAXFDS'th
entry in the fds array can be used.
---
src/lxc/cgroup.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
index a02ebc2..7bb88b5 100644
--- a/src/lxc/cgroup.c
+++ b/src/lxc/cgroup.c
@@ -398,7 +398,7 @@ int lxc_cgroup_prepare_attach(const char *name, void **data)
err = 0;
i = 0;
while ((mntent = getmntent(file))) {
- if (i >= MAXFDS - 1) {
+ if (i >= MAXFDS) {
ERROR("too many cgroups to attach to, aborting");
lxc_cgroup_dispose_attach(fds);
errno = ENOMEM;
--
1.7.1
--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
___
Lxc-devel mailing list
Lxc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lxc-devel
[lxc-devel] [PATCH 2/2] allow short -h and -n options to lxc-ps
--- doc/lxc-ps.sgml.in |4 ++-- src/lxc/lxc-ps.in |4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/lxc-ps.sgml.in b/doc/lxc-ps.sgml.in index f3275ae..401a17c 100644 --- a/doc/lxc-ps.sgml.in +++ b/doc/lxc-ps.sgml.in @@ -51,7 +51,7 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA lxc-ps --name name --lxc - ps option + -- ps option @@ -81,7 +81,7 @@ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - --name name + -n, --name name diff --git a/src/lxc/lxc-ps.in b/src/lxc/lxc-ps.in index a9923f0..4180aae 100644 --- a/src/lxc/lxc-ps.in +++ b/src/lxc/lxc-ps.in @@ -74,9 +74,9 @@ containers="" list_container_processes=0 for i in "$@"; do case $i in - --help) + -h|--help) help; exit 1;; - --name) + -n|--name) containers=$2; list_container_processes=1; shift 2;; --lxc) list_container_processes=1; shift;; -- 1.7.1 -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ Lxc-devel mailing list Lxc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 1/2] fix trivial off by one error
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
--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
___
Lxc-devel mailing list
Lxc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 1/2] fix trivial off by one error
On Tue, 18 Sep 2012 22:18:03 +0200
Christian Seiler 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 :)
--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
___
Lxc-devel mailing list
Lxc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lxc-devel
Re: [lxc-devel] [PATCH 1/2] fix trivial off by one error
Hi, > 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. I don't think performance is that much of an issue here, but to me it seems that using fd_set logic would complicate things quite a bit unnecessarily. The current logic is already a bit complicated because the cgroup task files have to be opened before setns() but written to only after the fork() call when we know the pid which happens after setns(). Having a simple array with a loop over it appears to be much more straight-forward to me, especially since iterating over an fd_set is kind of convoluted. > Or I can just add a comment :) My suggestion would be to do just that unless someone has a good reason to change the current logic. All IMHO of course, I just wrote the initial patch, in the end other people get to decide what goes in. ;-) Regards, Christian -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ Lxc-devel mailing list Lxc-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/lxc-devel
