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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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);
>