[lxc-devel] [PATCH] close correct side of the pipe and increase buffer size by one to handle \n

[Serge Hallyn]

Quoting S.Çağlar Onur (caglar at 10ur.org):
> Hey Serge,
> 
> On Mon, Feb 17, 2014 at 6:03 PM, Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
> > Quoting S.Çağlar Onur (caglar at 10ur.org):
> >> Signed-off-by: S.Çağlar Onur <caglar at 10ur.org>
> >
> > Thanks!  One comment,
> >
> >> ---
> >>  src/lxc/conf.c | 9 +++++----
> >>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/lxc/conf.c b/src/lxc/conf.c
> >> index 10f46ae..175a82f 100644
> >> --- a/src/lxc/conf.c
> >> +++ b/src/lxc/conf.c
> >> @@ -3011,13 +3011,14 @@ void lxc_delete_network(struct lxc_handler *handler)
> >>
> >>  #define LXC_USERNIC_PATH LIBEXECDIR "/lxc/lxc-user-nic"
> >>
> >> +/* lxc-user-nic returns "interface_name:interface_name\n" */
> >> +#define MAX_BUFFER_SIZE IFNAMSIZ*2 + 2
> >>  static int unpriv_assign_nic(struct lxc_netdev *netdev, pid_t pid)
> >>  {
> >>       pid_t child;
> >>       int bytes, pipefd[2];
> >>       char *token, *saveptr = NULL;
> >> -     /* lxc-user-nic returns "interface_name:interface_name" format */
> >> -     char buffer[IFNAMSIZ*2 + 1];
> >> +     char buffer[MAX_BUFFER_SIZE];
> >>
> >>       if (netdev->type != LXC_NET_VETH) {
> >>               ERROR("nic type %d not support for unprivileged use",
> >> @@ -3043,7 +3044,7 @@ static int unpriv_assign_nic(struct lxc_netdev *netdev, pid_t pid)
> >>               /* redirect the stdout to write-end of the pipe */
> >>               dup2(pipefd[1], STDOUT_FILENO);
> >>               /* close the write-end of the pipe */
> >> -             close(pipefd[0]);
> >> +             close(pipefd[1]);
> >>
> >>               // Call lxc-user-nic pid type bridge
> >>               char pidstr[20];
> >> @@ -3058,7 +3059,7 @@ static int unpriv_assign_nic(struct lxc_netdev *netdev, pid_t pid)
> >>       /* close the write-end of the pipe */
> >>       close(pipefd[1]);
> >>
> >> -     bytes = read(pipefd[0], &buffer, IFNAMSIZ*2 + 1);
> >> +     bytes = read(pipefd[0], &buffer, MAX_BUFFER_SIZE);
> >
> > Now technically this could mess us up, since the last byte we
> > read may not be 0.
> >
> > So for the read call itself I think it would be better to
> > stick to IFNAMSIZ*2 + 1).  Am I thinking clearly?
> 
> I think (if I'm not missing something obvious) we are reading up to
> MAX_BUFFER_SIZE (which is "IFNAMSIZ:IFNAMSIZ\n") and then placing \0
> to end of the the buffer via next line (which is not shown here but
> looks like this buffer[bytes - 1] = '\0';). Do you see a problem
> there?

Sorry - you're right.  It didnt' show up in yoru context and
I thought the buf was being memset earlier rather than len-1
being set to 0 at end.

It's already been applied, but in any case

Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>

thanks,
-serge