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

[S.Çağlar Onur]

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?

>>       if (bytes < 0) {
>>               SYSERROR("read failed");
>>       }
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> lxc-devel mailing list
>> lxc-devel at lists.linuxcontainers.org
>> http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel



-- 
S.Çağlar Onur <caglar at 10ur.org>