[PATCH for-4.5 0/3] Coverity fixes for python lowlevel libraries

[PATCH for-4.5 0/3] Coverity fixes for python lowlevel libraries

[Andrew Cooper]

These were initially flagged by the XenServer coverity scanning.  It turns out
that they were being found by the upstream scan, but had been grouped with
Xend and listed as ignored, hiding them from the output.  I have adjusted the
scan filters to not ignore the lowlevel libraries, and identified the
appropriate scan CIDs in the patches

While Xend is certainly dead and gone, XenServer at the very least still has
consumers of these python libraries.

Konrad: I am requesting a release ack for this.  All 5 issues are bugs with
the handling of error cases, rather than with the basic functionality
provided.  With these changes, Coverity is of the opinion that the python
libraries are perfect (0 issues), and I feel this is a worthy position to be
in for 4.5

Andrew Cooper (3):
  python/xc: Fix multiple issues in pyflask_context_to_sid()
  python/xc: Fix multiple issues in pyxc_readconsolering()
  python/xs: Correct the indirection of the NULL xshandle() check

 tools/python/xen/lowlevel/xc/xc.c |   34 +++++++++-------------------------
 tools/python/xen/lowlevel/xs/xs.c |    2 +-
 2 files changed, 10 insertions(+), 26 deletions(-)

[PATCH for-4.5 1/3] python/xc: Fix multiple issues in pyflask_context_to_sid()

[Andrew Cooper]

The error handling from a failed memory allocation should return
PyErr_SetFromErrno(xc_error_obj); rather than simply calling it and continuing
to the memcpy() below, with the dest pointer being NULL.

Furthermore, the context string is simply an input parameter to the hypercall,
and is not mutated anywhere along the way.  The error handling elsewhere in
the function can be simplified by not duplicating it to start with.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Coverity-IDs: 1055305 1055721
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Xen Coverity Team <coverity@xen.org>
---
 tools/python/xen/lowlevel/xc/xc.c |   21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index d95d459..c70b388 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -2126,8 +2126,6 @@ static PyObject *pyflask_context_to_sid(PyObject *self, PyObject *args,
 {
     xc_interface *xc_handle;
     char *ctx;
-    char *buf;
-    uint32_t len;
     uint32_t sid;
     int ret;
 
@@ -2137,28 +2135,15 @@ static PyObject *pyflask_context_to_sid(PyObject *self, PyObject *args,
                                       &ctx) )
         return NULL;
 
-    len = strlen(ctx);
-
-    buf = malloc(len);
-    if (!buf) {
-        errno = -ENOMEM;
-        PyErr_SetFromErrno(xc_error_obj);
-    }
-    
-    memcpy(buf, ctx, len);
-    
     xc_handle = xc_interface_open(0,0,0);
     if (!xc_handle) {
-        free(buf);
         return PyErr_SetFromErrno(xc_error_obj);
     }
-    
-    ret = xc_flask_context_to_sid(xc_handle, buf, len, &sid);
-        
+
+    ret = xc_flask_context_to_sid(xc_handle, ctx, strlen(ctx), &sid);
+
     xc_interface_close(xc_handle);
 
-    free(buf);
-    
     if ( ret != 0 ) {
         errno = -ret;
         return PyErr_SetFromErrno(xc_error_obj);
-- 
1.7.10.4

[PATCH for-4.5 2/3] python/xc: Fix multiple issues in pyxc_readconsolering()

[Andrew Cooper]

Don't leak a 16k allocation if PyArg_ParseTupleAndKeywords() or the first
xc_readconsolering() fail.  It is trivial to run throught the processes memory
by repeatedly passing junk parameters to this function.

In the case that the call to xc_readconsolering() in the while loop fails,
reinstate str before breaking out, and passing a spurious pointer to free().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Coverity-IDs: 1054984 1055906
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Xen Coverity Team <coverity@xen.org>
---
 tools/python/xen/lowlevel/xc/xc.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index c70b388..2aa0dc7 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -1089,7 +1089,7 @@ static PyObject *pyxc_readconsolering(XcObject *self,
 {
     unsigned int clear = 0, index = 0, incremental = 0;
     unsigned int count = 16384 + 1, size = count;
-    char        *str = malloc(size), *ptr;
+    char        *str, *ptr;
     PyObject    *obj;
     int          ret;
 
@@ -1097,15 +1097,17 @@ static PyObject *pyxc_readconsolering(XcObject *self,
 
     if ( !PyArg_ParseTupleAndKeywords(args, kwds, "|iii", kwd_list,
                                       &clear, &index, &incremental) ||
-         !str )
+         !(str = malloc(size)) )
         return NULL;
 
     ret = xc_readconsolering(self->xc_handle, str, &count, clear,
                              incremental, &index);
-    if ( ret < 0 )
+    if ( ret < 0 ) {
+        free(str);
         return pyxc_error_to_exception(self->xc_handle);
+    }
 
-    while ( !incremental && count == size )
+    while ( !incremental && count == size && ret >= 0 )
     {
         size += count - 1;
         if ( size < count )
@@ -1119,9 +1121,6 @@ static PyObject *pyxc_readconsolering(XcObject *self,
         count = size - count;
         ret = xc_readconsolering(self->xc_handle, str, &count, clear,
                                  1, &index);
-        if ( ret < 0 )
-            break;
-
         count += str - ptr;
         str = ptr;
     }
-- 
1.7.10.4

[PATCH for-4.5 3/3] python/xs: Correct the indirection of the NULL xshandle() check

[Andrew Cooper]

The code now now matches its comment, and will actually catch the case of a
bad xs handle.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Coverity-ID: 1055948
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Xen Coverity Team <coverity@xen.org>
---
 tools/python/xen/lowlevel/xs/xs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/python/xen/lowlevel/xs/xs.c b/tools/python/xen/lowlevel/xs/xs.c
index 84e1711..ec364bb 100644
--- a/tools/python/xen/lowlevel/xs/xs.c
+++ b/tools/python/xen/lowlevel/xs/xs.c
@@ -816,7 +816,7 @@ static int parse_transaction_path(XsHandle *self, PyObject *args,
 
     *xh = xshandle(self);
 
-    if (!xh)
+    if (!*xh)
         return 0;
 
     if (!PyArg_ParseTuple(args, "ss", &thstr, path))
-- 
1.7.10.4

Re: [PATCH for-4.5 3/3] python/xs: Correct the indirection of the NULL xshandle() check

[Ian Campbell]

On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:
> The code now now matches its comment, and will actually catch the case of a
> bad xs handle.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Coverity-ID: 1055948
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Xen Coverity Team <coverity@xen.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  tools/python/xen/lowlevel/xs/xs.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/python/xen/lowlevel/xs/xs.c b/tools/python/xen/lowlevel/xs/xs.c
> index 84e1711..ec364bb 100644
> --- a/tools/python/xen/lowlevel/xs/xs.c
> +++ b/tools/python/xen/lowlevel/xs/xs.c
> @@ -816,7 +816,7 @@ static int parse_transaction_path(XsHandle *self, PyObject *args,
>  
>      *xh = xshandle(self);
>  
> -    if (!xh)
> +    if (!*xh)
>          return 0;
>  
>      if (!PyArg_ParseTuple(args, "ss", &thstr, path))

Re: [PATCH for-4.5 1/3] python/xc: Fix multiple issues in pyflask_context_to_sid()

[Ian Campbell]

On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:
> The error handling from a failed memory allocation should return
> PyErr_SetFromErrno(xc_error_obj); rather than simply calling it and continuing
> to the memcpy() below, with the dest pointer being NULL.
> 
> Furthermore, the context string is simply an input parameter to the hypercall,
> and is not mutated anywhere along the way.  The error handling elsewhere in
> the function can be simplified by not duplicating it to start with.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Coverity-IDs: 1055305 1055721
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Xen Coverity Team <coverity@xen.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

This would have been far more obviously correct for 4.5 if you had stuck
to fixing the issue in the first paragraph.

> ---
>  tools/python/xen/lowlevel/xc/xc.c |   21 +++------------------
>  1 file changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
> index d95d459..c70b388 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -2126,8 +2126,6 @@ static PyObject *pyflask_context_to_sid(PyObject *self, PyObject *args,
>  {
>      xc_interface *xc_handle;
>      char *ctx;
> -    char *buf;
> -    uint32_t len;
>      uint32_t sid;
>      int ret;
>  
> @@ -2137,28 +2135,15 @@ static PyObject *pyflask_context_to_sid(PyObject *self, PyObject *args,
>                                        &ctx) )
>          return NULL;
>  
> -    len = strlen(ctx);
> -
> -    buf = malloc(len);
> -    if (!buf) {
> -        errno = -ENOMEM;
> -        PyErr_SetFromErrno(xc_error_obj);
> -    }
> -    
> -    memcpy(buf, ctx, len);
> -    
>      xc_handle = xc_interface_open(0,0,0);
>      if (!xc_handle) {
> -        free(buf);
>          return PyErr_SetFromErrno(xc_error_obj);
>      }
> -    
> -    ret = xc_flask_context_to_sid(xc_handle, buf, len, &sid);
> -        
> +
> +    ret = xc_flask_context_to_sid(xc_handle, ctx, strlen(ctx), &sid);
> +
>      xc_interface_close(xc_handle);
>  
> -    free(buf);
> -    
>      if ( ret != 0 ) {
>          errno = -ret;
>          return PyErr_SetFromErrno(xc_error_obj);

Re: [PATCH for-4.5 2/3] python/xc: Fix multiple issues in pyxc_readconsolering()

[Ian Campbell]

On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:
> Don't leak a 16k allocation if PyArg_ParseTupleAndKeywords() or the first
> xc_readconsolering() fail.  It is trivial to run throught the processes memory
> by repeatedly passing junk parameters to this function.
> 
> In the case that the call to xc_readconsolering() in the while loop fails,
> reinstate str before breaking out, and passing a spurious pointer to free().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Coverity-IDs: 1054984 1055906
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Xen Coverity Team <coverity@xen.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  tools/python/xen/lowlevel/xc/xc.c |   13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
> index c70b388..2aa0dc7 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -1089,7 +1089,7 @@ static PyObject *pyxc_readconsolering(XcObject *self,
>  {
>      unsigned int clear = 0, index = 0, incremental = 0;
>      unsigned int count = 16384 + 1, size = count;
> -    char        *str = malloc(size), *ptr;
> +    char        *str, *ptr;
>      PyObject    *obj;
>      int          ret;
>  
> @@ -1097,15 +1097,17 @@ static PyObject *pyxc_readconsolering(XcObject *self,
>  
>      if ( !PyArg_ParseTupleAndKeywords(args, kwds, "|iii", kwd_list,
>                                        &clear, &index, &incremental) ||
> -         !str )
> +         !(str = malloc(size)) )
>          return NULL;
>  
>      ret = xc_readconsolering(self->xc_handle, str, &count, clear,
>                               incremental, &index);
> -    if ( ret < 0 )
> +    if ( ret < 0 ) {
> +        free(str);
>          return pyxc_error_to_exception(self->xc_handle);
> +    }
>  
> -    while ( !incremental && count == size )
> +    while ( !incremental && count == size && ret >= 0 )
>      {
>          size += count - 1;
>          if ( size < count )
> @@ -1119,9 +1121,6 @@ static PyObject *pyxc_readconsolering(XcObject *self,
>          count = size - count;
>          ret = xc_readconsolering(self->xc_handle, str, &count, clear,
>                                   1, &index);
> -        if ( ret < 0 )
> -            break;
> -
>          count += str - ptr;
>          str = ptr;
>      }

Re: [PATCH for-4.5 1/3] python/xc: Fix multiple issues in pyflask_context_to_sid()

[Andrew Cooper]

On 28/11/14 11:37, Ian Campbell wrote:
> On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:
>> The error handling from a failed memory allocation should return
>> PyErr_SetFromErrno(xc_error_obj); rather than simply calling it and continuing
>> to the memcpy() below, with the dest pointer being NULL.
>>
>> Furthermore, the context string is simply an input parameter to the hypercall,
>> and is not mutated anywhere along the way.  The error handling elsewhere in
>> the function can be simplified by not duplicating it to start with.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Coverity-IDs: 1055305 1055721
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Xen Coverity Team <coverity@xen.org>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> This would have been far more obviously correct for 4.5 if you had stuck
> to fixing the issue in the first paragraph.

Hmm - I appear to have missed a detail in the description.  One of the
CIDs is to do with putting a string in a new buffer without a NUL
terminator, and passing it as a char* into xc_flask_context_to_sid; the
issue being that anyone expecting things like strlen() to work will be
in for a nasty shock.

One solution to this was strdup(), but as the duplication was
unnecessary anyway, it was easier just to drop it all.

~Andrew

>
>> ---
>>  tools/python/xen/lowlevel/xc/xc.c |   21 +++------------------
>>  1 file changed, 3 insertions(+), 18 deletions(-)
>>
>> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
>> index d95d459..c70b388 100644
>> --- a/tools/python/xen/lowlevel/xc/xc.c
>> +++ b/tools/python/xen/lowlevel/xc/xc.c
>> @@ -2126,8 +2126,6 @@ static PyObject *pyflask_context_to_sid(PyObject *self, PyObject *args,
>>  {
>>      xc_interface *xc_handle;
>>      char *ctx;
>> -    char *buf;
>> -    uint32_t len;
>>      uint32_t sid;
>>      int ret;
>>  
>> @@ -2137,28 +2135,15 @@ static PyObject *pyflask_context_to_sid(PyObject *self, PyObject *args,
>>                                        &ctx) )
>>          return NULL;
>>  
>> -    len = strlen(ctx);
>> -
>> -    buf = malloc(len);
>> -    if (!buf) {
>> -        errno = -ENOMEM;
>> -        PyErr_SetFromErrno(xc_error_obj);
>> -    }
>> -    
>> -    memcpy(buf, ctx, len);
>> -    
>>      xc_handle = xc_interface_open(0,0,0);
>>      if (!xc_handle) {
>> -        free(buf);
>>          return PyErr_SetFromErrno(xc_error_obj);
>>      }
>> -    
>> -    ret = xc_flask_context_to_sid(xc_handle, buf, len, &sid);
>> -        
>> +
>> +    ret = xc_flask_context_to_sid(xc_handle, ctx, strlen(ctx), &sid);
>> +
>>      xc_interface_close(xc_handle);
>>  
>> -    free(buf);
>> -    
>>      if ( ret != 0 ) {
>>          errno = -ret;
>>          return PyErr_SetFromErrno(xc_error_obj);
>

Re: [PATCH for-4.5 0/3] Coverity fixes for python lowlevel libraries

[Ian Campbell]

On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:

> While Xend is certainly dead and gone, XenServer at the very least still has
> consumers of these python libraries.

AIUI XS mainly uses the xs.c stuff, with the xc.c stuff being mainly
debug utilities.

This is important because while the xs.c is simple and obviously correct
the xc.c patches are a little more involved.

> 
> Konrad: I am requesting a release ack for this.  All 5 issues are bugs with
> the handling of error cases, rather than with the basic functionality
> provided.  With these changes, Coverity is of the opinion that the python
> libraries are perfect (0 issues), and I feel this is a worthy position to be
> in for 4.5
> 
> Andrew Cooper (3):
>   python/xc: Fix multiple issues in pyflask_context_to_sid()
>   python/xc: Fix multiple issues in pyxc_readconsolering()
>   python/xs: Correct the indirection of the NULL xshandle() check
> 
>  tools/python/xen/lowlevel/xc/xc.c |   34 +++++++++-------------------------
>  tools/python/xen/lowlevel/xs/xs.c |    2 +-
>  2 files changed, 10 insertions(+), 26 deletions(-)
> 

Re: [PATCH for-4.5 1/3] python/xc: Fix multiple issues in pyflask_context_to_sid()

[Ian Campbell]

On Fri, 2014-11-28 at 11:47 +0000, Andrew Cooper wrote:
> On 28/11/14 11:37, Ian Campbell wrote:
> > On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:
> >> The error handling from a failed memory allocation should return
> >> PyErr_SetFromErrno(xc_error_obj); rather than simply calling it and continuing
> >> to the memcpy() below, with the dest pointer being NULL.
> >>
> >> Furthermore, the context string is simply an input parameter to the hypercall,
> >> and is not mutated anywhere along the way.  The error handling elsewhere in
> >> the function can be simplified by not duplicating it to start with.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> Coverity-IDs: 1055305 1055721
> >> CC: Ian Campbell <Ian.Campbell@citrix.com>
> >> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> >> CC: Wei Liu <wei.liu2@citrix.com>
> >> CC: Xen Coverity Team <coverity@xen.org>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >
> > This would have been far more obviously correct for 4.5 if you had stuck
> > to fixing the issue in the first paragraph.
> 
> Hmm - I appear to have missed a detail in the description.  One of the
> CIDs is to do with putting a string in a new buffer without a NUL
> terminator, and passing it as a char* into xc_flask_context_to_sid; the
> issue being that anyone expecting things like strlen() to work will be
> in for a nasty shock.

ISTR a discussion of this interface in Julien's device-tree passthrough
thing a while back and some sort of conclusion that the hypervisor was
supposed to use the len field and not rely on the NULL.

I'm not sure that necessarily invalidates what you are saying, since
even given that throwing a NULL on the end would be friendly to libxc
consumers if nothing else.

Ian.

Re: [PATCH for-4.5 3/3] python/xs: Correct the indirection of the NULL xshandle() check

[Konrad Rzeszutek Wilk]

On Thu, Nov 27, 2014 at 12:34:34PM +0000, Andrew Cooper wrote:
> The code now now matches its comment, and will actually catch the case of a
> bad xs handle.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Coverity-ID: 1055948
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Xen Coverity Team <coverity@xen.org>

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/python/xen/lowlevel/xs/xs.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/python/xen/lowlevel/xs/xs.c b/tools/python/xen/lowlevel/xs/xs.c
> index 84e1711..ec364bb 100644
> --- a/tools/python/xen/lowlevel/xs/xs.c
> +++ b/tools/python/xen/lowlevel/xs/xs.c
> @@ -816,7 +816,7 @@ static int parse_transaction_path(XsHandle *self, PyObject *args,
>  
>      *xh = xshandle(self);
>  
> -    if (!xh)
> +    if (!*xh)
>          return 0;
>  
>      if (!PyArg_ParseTuple(args, "ss", &thstr, path))
> -- 
> 1.7.10.4
> 

Re: [PATCH for-4.5 2/3] python/xc: Fix multiple issues in pyxc_readconsolering()

[Konrad Rzeszutek Wilk]

On Fri, Nov 28, 2014 at 11:38:52AM +0000, Ian Campbell wrote:
> On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:
> > Don't leak a 16k allocation if PyArg_ParseTupleAndKeywords() or the first
> > xc_readconsolering() fail.  It is trivial to run throught the processes memory
> > by repeatedly passing junk parameters to this function.
> > 
> > In the case that the call to xc_readconsolering() in the while loop fails,
> > reinstate str before breaking out, and passing a spurious pointer to free().
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Coverity-IDs: 1054984 1055906
> > CC: Ian Campbell <Ian.Campbell@citrix.com>
> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > CC: Xen Coverity Team <coverity@xen.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> > ---
> >  tools/python/xen/lowlevel/xc/xc.c |   13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
> > index c70b388..2aa0dc7 100644
> > --- a/tools/python/xen/lowlevel/xc/xc.c
> > +++ b/tools/python/xen/lowlevel/xc/xc.c
> > @@ -1089,7 +1089,7 @@ static PyObject *pyxc_readconsolering(XcObject *self,
> >  {
> >      unsigned int clear = 0, index = 0, incremental = 0;
> >      unsigned int count = 16384 + 1, size = count;
> > -    char        *str = malloc(size), *ptr;
> > +    char        *str, *ptr;
> >      PyObject    *obj;
> >      int          ret;
> >  
> > @@ -1097,15 +1097,17 @@ static PyObject *pyxc_readconsolering(XcObject *self,
> >  
> >      if ( !PyArg_ParseTupleAndKeywords(args, kwds, "|iii", kwd_list,
> >                                        &clear, &index, &incremental) ||
> > -         !str )
> > +         !(str = malloc(size)) )
> >          return NULL;
> >  
> >      ret = xc_readconsolering(self->xc_handle, str, &count, clear,
> >                               incremental, &index);
> > -    if ( ret < 0 )
> > +    if ( ret < 0 ) {
> > +        free(str);
> >          return pyxc_error_to_exception(self->xc_handle);
> > +    }
> >  
> > -    while ( !incremental && count == size )
> > +    while ( !incremental && count == size && ret >= 0 )
> >      {
> >          size += count - 1;
> >          if ( size < count )
> > @@ -1119,9 +1121,6 @@ static PyObject *pyxc_readconsolering(XcObject *self,
> >          count = size - count;
> >          ret = xc_readconsolering(self->xc_handle, str, &count, clear,
> >                                   1, &index);
> > -        if ( ret < 0 )
> > -            break;
> > -
> >          count += str - ptr;
> >          str = ptr;
> >      }
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH for-4.5 2/3] python/xc: Fix multiple issues in pyxc_readconsolering()

[Ian Campbell]

On Mon, 2014-12-01 at 16:14 -0500, Konrad Rzeszutek Wilk wrote:
> On Fri, Nov 28, 2014 at 11:38:52AM +0000, Ian Campbell wrote:
> > On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:
> > > Don't leak a 16k allocation if PyArg_ParseTupleAndKeywords() or the first
> > > xc_readconsolering() fail.  It is trivial to run throught the processes memory
> > > by repeatedly passing junk parameters to this function.
> > > 
> > > In the case that the call to xc_readconsolering() in the while loop fails,
> > > reinstate str before breaking out, and passing a spurious pointer to free().
> > > 
> > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > Coverity-IDs: 1054984 1055906
> > > CC: Ian Campbell <Ian.Campbell@citrix.com>
> > > CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > > CC: Wei Liu <wei.liu2@citrix.com>
> > > CC: Xen Coverity Team <coverity@xen.org>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Did you intend to also ack patch #1? (or have I missed a mail?)

Re: [PATCH for-4.5 2/3] python/xc: Fix multiple issues in pyxc_readconsolering()

[Konrad Rzeszutek Wilk]

On Tue, Dec 02, 2014 at 01:50:37PM +0000, Ian Campbell wrote:
> On Mon, 2014-12-01 at 16:14 -0500, Konrad Rzeszutek Wilk wrote:
> > On Fri, Nov 28, 2014 at 11:38:52AM +0000, Ian Campbell wrote:
> > > On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:
> > > > Don't leak a 16k allocation if PyArg_ParseTupleAndKeywords() or the first
> > > > xc_readconsolering() fail.  It is trivial to run throught the processes memory
> > > > by repeatedly passing junk parameters to this function.
> > > > 
> > > > In the case that the call to xc_readconsolering() in the while loop fails,
> > > > reinstate str before breaking out, and passing a spurious pointer to free().
> > > > 
> > > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > Coverity-IDs: 1054984 1055906
> > > > CC: Ian Campbell <Ian.Campbell@citrix.com>
> > > > CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > > > CC: Wei Liu <wei.liu2@citrix.com>
> > > > CC: Xen Coverity Team <coverity@xen.org>
> > > 
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Did you intend to also ack patch #1? (or have I missed a mail?)

No. You two still were discussing it so I figured I will wait
until a repost.
> 
> 
> 

Re: [PATCH for-4.5 2/3] python/xc: Fix multiple issues in pyxc_readconsolering()

[Ian Campbell]

On Tue, 2014-12-02 at 13:47 -0500, Konrad Rzeszutek Wilk wrote:
> On Tue, Dec 02, 2014 at 01:50:37PM +0000, Ian Campbell wrote:
> > On Mon, 2014-12-01 at 16:14 -0500, Konrad Rzeszutek Wilk wrote:
> > > On Fri, Nov 28, 2014 at 11:38:52AM +0000, Ian Campbell wrote:
> > > > On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:
> > > > > Don't leak a 16k allocation if PyArg_ParseTupleAndKeywords() or the first
> > > > > xc_readconsolering() fail.  It is trivial to run throught the processes memory
> > > > > by repeatedly passing junk parameters to this function.
> > > > > 
> > > > > In the case that the call to xc_readconsolering() in the while loop fails,
> > > > > reinstate str before breaking out, and passing a spurious pointer to free().
> > > > > 
> > > > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > > Coverity-IDs: 1054984 1055906
> > > > > CC: Ian Campbell <Ian.Campbell@citrix.com>
> > > > > CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > > > > CC: Wei Liu <wei.liu2@citrix.com>
> > > > > CC: Xen Coverity Team <coverity@xen.org>
> > > > 
> > > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > Did you intend to also ack patch #1? (or have I missed a mail?)
> 
> No. You two still were discussing it so I figured I will wait
> until a repost.

I've applied these two.

FWIW despite the discussion I think the first could go in too, and
probably doesn't need a resend.

Re: [PATCH for-4.5 1/3] python/xc: Fix multiple issues in pyflask_context_to_sid()

[Ian Campbell]

On Fri, 2014-11-28 at 11:37 +0000, Ian Campbell wrote:
> On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:
> > The error handling from a failed memory allocation should return
> > PyErr_SetFromErrno(xc_error_obj); rather than simply calling it and continuing
> > to the memcpy() below, with the dest pointer being NULL.
> > 
> > Furthermore, the context string is simply an input parameter to the hypercall,
> > and is not mutated anywhere along the way.  The error handling elsewhere in
> > the function can be simplified by not duplicating it to start with.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Coverity-IDs: 1055305 1055721
> > CC: Ian Campbell <Ian.Campbell@citrix.com>
> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > CC: Xen Coverity Team <coverity@xen.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> This would have been far more obviously correct for 4.5 if you had stuck
> to fixing the issue in the first paragraph.

Konrad, given
http://article.gmane.org/gmane.comp.emulators.xen.devel/224881 does this
have a release ack?

Ian.

Re: [PATCH for-4.5 1/3] python/xc: Fix multiple issues in pyflask_context_to_sid()

[Andrew Cooper]

On 09/12/14 14:27, Ian Campbell wrote:
> On Fri, 2014-11-28 at 11:37 +0000, Ian Campbell wrote:
>> On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:
>>> The error handling from a failed memory allocation should return
>>> PyErr_SetFromErrno(xc_error_obj); rather than simply calling it and continuing
>>> to the memcpy() below, with the dest pointer being NULL.
>>>
>>> Furthermore, the context string is simply an input parameter to the hypercall,
>>> and is not mutated anywhere along the way.  The error handling elsewhere in
>>> the function can be simplified by not duplicating it to start with.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Coverity-IDs: 1055305 1055721
>>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>> CC: Xen Coverity Team <coverity@xen.org>
>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>
>> This would have been far more obviously correct for 4.5 if you had stuck
>> to fixing the issue in the first paragraph.
> Konrad, given
> http://article.gmane.org/gmane.comp.emulators.xen.devel/224881 does this
> have a release ack?
>
> Ian.
>

I can resubmit with a clearer description if that would help clarity,
but the code is correct for the fixes (not fantastically well) described.

~Andrew

Re: [PATCH for-4.5 1/3] python/xc: Fix multiple issues in pyflask_context_to_sid()

[Konrad Rzeszutek Wilk]

On Tue, Dec 09, 2014 at 02:30:24PM +0000, Andrew Cooper wrote:
> On 09/12/14 14:27, Ian Campbell wrote:
> > On Fri, 2014-11-28 at 11:37 +0000, Ian Campbell wrote:
> >> On Thu, 2014-11-27 at 12:34 +0000, Andrew Cooper wrote:
> >>> The error handling from a failed memory allocation should return
> >>> PyErr_SetFromErrno(xc_error_obj); rather than simply calling it and continuing
> >>> to the memcpy() below, with the dest pointer being NULL.
> >>>
> >>> Furthermore, the context string is simply an input parameter to the hypercall,
> >>> and is not mutated anywhere along the way.  The error handling elsewhere in
> >>> the function can be simplified by not duplicating it to start with.
> >>>
> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>> Coverity-IDs: 1055305 1055721
> >>> CC: Ian Campbell <Ian.Campbell@citrix.com>
> >>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> >>> CC: Wei Liu <wei.liu2@citrix.com>
> >>> CC: Xen Coverity Team <coverity@xen.org>
> >> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >>
> >> This would have been far more obviously correct for 4.5 if you had stuck
> >> to fixing the issue in the first paragraph.
> > Konrad, given
> > http://article.gmane.org/gmane.comp.emulators.xen.devel/224881 does this
> > have a release ack?
> >
> > Ian.
> >
> 
> I can resubmit with a clearer description if that would help clarity,
> but the code is correct for the fixes (not fantastically well) described.

Please do - that is all I was waiting for. Thank you.
> 
> ~Andrew

[PATCH v2 for-4.5 1/3] python/xc: Fix multiple issues in pyflask_context_to_sid()

[Andrew Cooper]

The error handling from a failed memory allocation should return
PyErr_SetFromErrno(xc_error_obj); rather than simply calling it and continuing
to the memcpy() below, with the dest pointer being NULL.

Coverity also complains about passing a non-NUL terminated string to
xc_flask_context_to_sid().  xc_flask_context_to_sid() doesn't actually take a
NUL terminated string, but it does take a char* which, in context, used to be
a string, which is why Coverity complains.

One solution would be to use strdup(ctx) which is simpler than a
strlen()/malloc()/memcpy() combo, which would result in a NUL-terminated
string being used with xc_flask_context_to_sid().

However, ctx is strictly an input to the hypercall and is not mutated along
the way.  Both these issues can be fixed, and the error logic simplified, by
not duplicating ctx in the first place.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Coverity-IDs: 1055305 1055721
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Xen Coverity Team <coverity@xen.org>

---
v2: Expand the commit message.  No code change
---
 tools/python/xen/lowlevel/xc/xc.c |   21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index f83e33d..2aa0dc7 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -2125,8 +2125,6 @@ static PyObject *pyflask_context_to_sid(PyObject *self, PyObject *args,
 {
     xc_interface *xc_handle;
     char *ctx;
-    char *buf;
-    uint32_t len;
     uint32_t sid;
     int ret;
 
@@ -2136,28 +2134,15 @@ static PyObject *pyflask_context_to_sid(PyObject *self, PyObject *args,
                                       &ctx) )
         return NULL;
 
-    len = strlen(ctx);
-
-    buf = malloc(len);
-    if (!buf) {
-        errno = -ENOMEM;
-        PyErr_SetFromErrno(xc_error_obj);
-    }
-    
-    memcpy(buf, ctx, len);
-    
     xc_handle = xc_interface_open(0,0,0);
     if (!xc_handle) {
-        free(buf);
         return PyErr_SetFromErrno(xc_error_obj);
     }
-    
-    ret = xc_flask_context_to_sid(xc_handle, buf, len, &sid);
-        
+
+    ret = xc_flask_context_to_sid(xc_handle, ctx, strlen(ctx), &sid);
+
     xc_interface_close(xc_handle);
 
-    free(buf);
-    
     if ( ret != 0 ) {
         errno = -ret;
         return PyErr_SetFromErrno(xc_error_obj);
-- 
1.7.10.4

Re: [PATCH v2 for-4.5 1/3] python/xc: Fix multiple issues in pyflask_context_to_sid()

[Konrad Rzeszutek Wilk]

On Tue, Dec 09, 2014 at 04:43:22PM +0000, Andrew Cooper wrote:
> The error handling from a failed memory allocation should return
> PyErr_SetFromErrno(xc_error_obj); rather than simply calling it and continuing
> to the memcpy() below, with the dest pointer being NULL.
> 
> Coverity also complains about passing a non-NUL terminated string to
> xc_flask_context_to_sid().  xc_flask_context_to_sid() doesn't actually take a
> NUL terminated string, but it does take a char* which, in context, used to be
> a string, which is why Coverity complains.
> 
> One solution would be to use strdup(ctx) which is simpler than a
> strlen()/malloc()/memcpy() combo, which would result in a NUL-terminated
> string being used with xc_flask_context_to_sid().
> 
> However, ctx is strictly an input to the hypercall and is not mutated along
> the way.  Both these issues can be fixed, and the error logic simplified, by
> not duplicating ctx in the first place.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Coverity-IDs: 1055305 1055721
> Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Xen Coverity Team <coverity@xen.org>
> 
> ---
> v2: Expand the commit message.  No code change

Thank you.

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> ---
>  tools/python/xen/lowlevel/xc/xc.c |   21 +++------------------
>  1 file changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
> index f83e33d..2aa0dc7 100644
> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -2125,8 +2125,6 @@ static PyObject *pyflask_context_to_sid(PyObject *self, PyObject *args,
>  {
>      xc_interface *xc_handle;
>      char *ctx;
> -    char *buf;
> -    uint32_t len;
>      uint32_t sid;
>      int ret;
>  
> @@ -2136,28 +2134,15 @@ static PyObject *pyflask_context_to_sid(PyObject *self, PyObject *args,
>                                        &ctx) )
>          return NULL;
>  
> -    len = strlen(ctx);
> -
> -    buf = malloc(len);
> -    if (!buf) {
> -        errno = -ENOMEM;
> -        PyErr_SetFromErrno(xc_error_obj);
> -    }
> -    
> -    memcpy(buf, ctx, len);
> -    
>      xc_handle = xc_interface_open(0,0,0);
>      if (!xc_handle) {
> -        free(buf);
>          return PyErr_SetFromErrno(xc_error_obj);
>      }
> -    
> -    ret = xc_flask_context_to_sid(xc_handle, buf, len, &sid);
> -        
> +
> +    ret = xc_flask_context_to_sid(xc_handle, ctx, strlen(ctx), &sid);
> +
>      xc_interface_close(xc_handle);
>  
> -    free(buf);
> -    
>      if ( ret != 0 ) {
>          errno = -ret;
>          return PyErr_SetFromErrno(xc_error_obj);
> -- 
> 1.7.10.4
> 

Re: [PATCH v2 for-4.5 1/3] python/xc: Fix multiple issues in pyflask_context_to_sid()

[Ian Campbell]

On Wed, 2014-12-10 at 12:13 -0500, Konrad Rzeszutek Wilk wrote:
> On Tue, Dec 09, 2014 at 04:43:22PM +0000, Andrew Cooper wrote:
> > The error handling from a failed memory allocation should return
> > PyErr_SetFromErrno(xc_error_obj); rather than simply calling it and continuing
> > to the memcpy() below, with the dest pointer being NULL.
> > 
> > Coverity also complains about passing a non-NUL terminated string to
> > xc_flask_context_to_sid().  xc_flask_context_to_sid() doesn't actually take a
> > NUL terminated string, but it does take a char* which, in context, used to be
> > a string, which is why Coverity complains.
> > 
> > One solution would be to use strdup(ctx) which is simpler than a
> > strlen()/malloc()/memcpy() combo, which would result in a NUL-terminated
> > string being used with xc_flask_context_to_sid().
> > 
> > However, ctx is strictly an input to the hypercall and is not mutated along
> > the way.  Both these issues can be fixed, and the error logic simplified, by
> > not duplicating ctx in the first place.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Coverity-IDs: 1055305 1055721
> > Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
> > CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > CC: Xen Coverity Team <coverity@xen.org>
> > 
> > ---
> > v2: Expand the commit message.  No code change
> 
> Thank you.
> 
> Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 

Applied.