util: set OOM in virCopyLastError if error is not set

[libvirt] [PATCH v2] util: set OOM in virCopyLastError if error is not set

Posted by Nikolay Shirokovskiy 5 years, 9 months ago

virCopyLastError is intended to be used after last error is set.
However due to virLastErrorObject failures (very unlikely through
as thread local error is allocated on first use) we can have zero
fields in a copy as a result. In particular code field can be set
to VIR_ERR_OK.

In some places (qemu monitor, qemu agent and qemu migaration code
for example) we use copy result as a flag and this leads to bugs.

Let's set OOM-like error in copy in case of virLastErrorObject failures.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---

Changes from v1:
- check @to
- set OMM error instead of using virErrorGenericFailure

 src/util/virerror.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/util/virerror.c b/src/util/virerror.c
index f198f27..737ea92 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -366,19 +366,25 @@ virSetError(virErrorPtr newerr)
  *
  * One will need to free the result with virResetError()
  *
- * Returns 0 if no error was found and the error code otherwise and -1 in case
- *         of parameter error.
+ * Returns error code or -1 in case of parameter error.
  */
 int
 virCopyLastError(virErrorPtr to)
 {
     virErrorPtr err = virLastErrorObject();
+
+    if (!to)
+        return -1;
+
     /* We can't guarantee caller has initialized it to zero */
     memset(to, 0, sizeof(*to));
-    if (err)
+    if (err) {
         virCopyError(err, to);
-    else
-        virResetError(to);
+    } else {
+        to->code = VIR_ERR_NO_MEMORY;
+        to->domain = VIR_FROM_NONE;
+        to->level = VIR_ERR_ERROR;
+    }
     return to->code;
 }
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] util: set OOM in virCopyLastError if error is not set

Posted by John Ferlan 5 years, 9 months ago


On 07/02/2018 07:33 AM, Nikolay Shirokovskiy wrote:
> virCopyLastError is intended to be used after last error is set.
> However due to virLastErrorObject failures (very unlikely through
> as thread local error is allocated on first use) we can have zero
> fields in a copy as a result. In particular code field can be set
> to VIR_ERR_OK.
> 
> In some places (qemu monitor, qemu agent and qemu migaration code
> for example) we use copy result as a flag and this leads to bugs.
> 
> Let's set OOM-like error in copy in case of virLastErrorObject failures.
> 
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> ---
> 
> Changes from v1:
> - check @to
> - set OMM error instead of using virErrorGenericFailure
> 
>  src/util/virerror.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/src/util/virerror.c b/src/util/virerror.c
> index f198f27..737ea92 100644
> --- a/src/util/virerror.c
> +++ b/src/util/virerror.c
> @@ -366,19 +366,25 @@ virSetError(virErrorPtr newerr)
>   *
>   * One will need to free the result with virResetError()
>   *
> - * Returns 0 if no error was found and the error code otherwise and -1 in case
> - *         of parameter error.
> + * Returns error code or -1 in case of parameter error.
>   */
>  int
>  virCopyLastError(virErrorPtr to)
>  {
>      virErrorPtr err = virLastErrorObject();
> +
> +    if (!to)
> +        return -1;
> +
>      /* We can't guarantee caller has initialized it to zero */
>      memset(to, 0, sizeof(*to));
> -    if (err)
> +    if (err) {
>          virCopyError(err, to);
> -    else
> -        virResetError(to);
> +    } else {
> +        to->code = VIR_ERR_NO_MEMORY;
> +        to->domain = VIR_FROM_NONE;
> +        to->level = VIR_ERR_ERROR;

Should we do a VIR_FREE(to->message); so that nothing that was there
before somehow remains... I don't think either agent or monitor
"lastError" is reset until Dispose time.

Beyond that, I think this works for this edge/odd/bad case...

Reviewed-by: John Ferlan <jferlan@redhat.com>

John

> +    }
>      return to->code;
>  }
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] util: set OOM in virCopyLastError if error is not set

Posted by Nikolay Shirokovskiy 5 years, 9 months ago


On 17.07.2018 01:38, John Ferlan wrote:
> 
> 
> On 07/02/2018 07:33 AM, Nikolay Shirokovskiy wrote:
>> virCopyLastError is intended to be used after last error is set.
>> However due to virLastErrorObject failures (very unlikely through
>> as thread local error is allocated on first use) we can have zero
>> fields in a copy as a result. In particular code field can be set
>> to VIR_ERR_OK.
>>
>> In some places (qemu monitor, qemu agent and qemu migaration code
>> for example) we use copy result as a flag and this leads to bugs.
>>
>> Let's set OOM-like error in copy in case of virLastErrorObject failures.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
>> ---
>>
>> Changes from v1:
>> - check @to
>> - set OMM error instead of using virErrorGenericFailure
>>
>>  src/util/virerror.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/util/virerror.c b/src/util/virerror.c
>> index f198f27..737ea92 100644
>> --- a/src/util/virerror.c
>> +++ b/src/util/virerror.c
>> @@ -366,19 +366,25 @@ virSetError(virErrorPtr newerr)
>>   *
>>   * One will need to free the result with virResetError()
>>   *
>> - * Returns 0 if no error was found and the error code otherwise and -1 in case
>> - *         of parameter error.
>> + * Returns error code or -1 in case of parameter error.
>>   */
>>  int
>>  virCopyLastError(virErrorPtr to)
>>  {
>>      virErrorPtr err = virLastErrorObject();
>> +
>> +    if (!to)
>> +        return -1;
>> +
>>      /* We can't guarantee caller has initialized it to zero */
>>      memset(to, 0, sizeof(*to));
>> -    if (err)
>> +    if (err) {
>>          virCopyError(err, to);
>> -    else
>> -        virResetError(to);
>> +    } else {
>> +        to->code = VIR_ERR_NO_MEMORY;
>> +        to->domain = VIR_FROM_NONE;
>> +        to->level = VIR_ERR_ERROR;
> 
> Should we do a VIR_FREE(to->message); so that nothing that was there
> before somehow remains... I don't think either agent or monitor> "lastError" is reset until Dispose time.

Won't hurt but probably will not be used by monitor or agent. If thread
error is not allocated message is NULL upon return, after error is allocated we never
hit this OOM branch anymore. Of course hypotetical client can bring @to
with message already set so this a bit future proof. 

I think then we can leave reset and then set these 3 fields.

Nikolay

> 
> Beyond that, I think this works for this edge/odd/bad case...
> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> John
> 
>> +    }
>>      return to->code;
>>  }
>>  
>>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] util: set OOM in virCopyLastError if error is not set

Posted by John Ferlan 5 years, 9 months ago


>>> +    } else {
>>> +        to->code = VIR_ERR_NO_MEMORY;
>>> +        to->domain = VIR_FROM_NONE;
>>> +        to->level = VIR_ERR_ERROR;
>>
>> Should we do a VIR_FREE(to->message); so that nothing that was there
>> before somehow remains... I don't think either agent or monitor> "lastError" is reset until Dispose time.
> 
> Won't hurt but probably will not be used by monitor or agent. If thread
> error is not allocated message is NULL upon return, after error is allocated we never
> hit this OOM branch anymore. Of course hypotetical client can bring @to
> with message already set so this a bit future proof. 
> 
> I think then we can leave reset and then set these 3 fields.
> 
> Nikolay
> 

You have commit access and my R-by regardless of whether you add the
VIR_FREE or not.  I leave the rest to you ;-)

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] util: set OOM in virCopyLastError if error is not set

Posted by Nikolay Shirokovskiy 5 years, 9 months ago


On 17.07.2018 22:28, John Ferlan wrote:
> 
> 
>>>> +    } else {
>>>> +        to->code = VIR_ERR_NO_MEMORY;
>>>> +        to->domain = VIR_FROM_NONE;
>>>> +        to->level = VIR_ERR_ERROR;
>>>
>>> Should we do a VIR_FREE(to->message); so that nothing that was there
>>> before somehow remains... I don't think either agent or monitor> "lastError" is reset until Dispose time.
>>
>> Won't hurt but probably will not be used by monitor or agent. If thread
>> error is not allocated message is NULL upon return, after error is allocated we never
>> hit this OOM branch anymore. Of course hypotetical client can bring @to
>> with message already set so this a bit future proof. 
>>
>> I think then we can leave reset and then set these 3 fields.
>>
>> Nikolay
>>
> 
> You have commit access and my R-by regardless of whether you add the
> VIR_FREE or not.  I leave the rest to you ;-)
> 

Thanx! Pushed with reset added.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] util: set OOM in virCopyLastError if error is not set

Posted by John Ferlan 5 years, 9 months ago


On 07/19/2018 04:15 AM, Nikolay Shirokovskiy wrote:
> 
> 
> On 17.07.2018 22:28, John Ferlan wrote:
>>
>>
>>>>> +    } else {
>>>>> +        to->code = VIR_ERR_NO_MEMORY;
>>>>> +        to->domain = VIR_FROM_NONE;
>>>>> +        to->level = VIR_ERR_ERROR;
>>>>
>>>> Should we do a VIR_FREE(to->message); so that nothing that was there
>>>> before somehow remains... I don't think either agent or monitor> "lastError" is reset until Dispose time.
>>>
>>> Won't hurt but probably will not be used by monitor or agent. If thread
>>> error is not allocated message is NULL upon return, after error is allocated we never
>>> hit this OOM branch anymore. Of course hypotetical client can bring @to
>>> with message already set so this a bit future proof. 
>>>
>>> I think then we can leave reset and then set these 3 fields.
>>>
>>> Nikolay
>>>
>>
>> You have commit access and my R-by regardless of whether you add the
>> VIR_FREE or not.  I leave the rest to you ;-)
>>
> 
> Thanx! Pushed with reset added.
> 
> Nikolay
> 

Oh no, you pushed a local .gnulib too!

git log -p
...
commit 1bff5bbe25eb7a7e7a4e0067c4ca7cbc1cb34999
Author: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Date:   Mon Jul 2 14:16:52 2018 +0300

...
diff --git a/.gnulib b/.gnulib
index cdbf3d385a..d6397dde2e 160000
--- a/.gnulib
+++ b/.gnulib
@@ -1 +1 @@
-Subproject commit cdbf3d385a32ff904c96f20c26f3470bd8345248
+Subproject commit d6397dde2e127e246e3eeb5254a21f42cac783c8
diff --git a/src/util/virerror.c b/src/util/virerror.c
index f198f27957..5f26d59777 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c


...

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] util: set OOM in virCopyLastError if error is not set

Posted by Nikolay Shirokovskiy 5 years, 9 months ago


On 19.07.2018 20:30, John Ferlan wrote:
> 
> 
> On 07/19/2018 04:15 AM, Nikolay Shirokovskiy wrote:
>>
>>
>> On 17.07.2018 22:28, John Ferlan wrote:
>>>
>>>
>>>>>> +    } else {
>>>>>> +        to->code = VIR_ERR_NO_MEMORY;
>>>>>> +        to->domain = VIR_FROM_NONE;
>>>>>> +        to->level = VIR_ERR_ERROR;
>>>>>
>>>>> Should we do a VIR_FREE(to->message); so that nothing that was there
>>>>> before somehow remains... I don't think either agent or monitor> "lastError" is reset until Dispose time.
>>>>
>>>> Won't hurt but probably will not be used by monitor or agent. If thread
>>>> error is not allocated message is NULL upon return, after error is allocated we never
>>>> hit this OOM branch anymore. Of course hypotetical client can bring @to
>>>> with message already set so this a bit future proof. 
>>>>
>>>> I think then we can leave reset and then set these 3 fields.
>>>>
>>>> Nikolay
>>>>
>>>
>>> You have commit access and my R-by regardless of whether you add the
>>> VIR_FREE or not.  I leave the rest to you ;-)
>>>
>>
>> Thanx! Pushed with reset added.
>>
>> Nikolay
>>
> 
> Oh no, you pushed a local .gnulib too!
> 
> git log -p
> ...
> commit 1bff5bbe25eb7a7e7a4e0067c4ca7cbc1cb34999
> Author: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> Date:   Mon Jul 2 14:16:52 2018 +0300
> 
> ...
> diff --git a/.gnulib b/.gnulib
> index cdbf3d385a..d6397dde2e 160000
> --- a/.gnulib
> +++ b/.gnulib
> @@ -1 +1 @@
> -Subproject commit cdbf3d385a32ff904c96f20c26f3470bd8345248
> +Subproject commit d6397dde2e127e246e3eeb5254a21f42cac783c8
> diff --git a/src/util/virerror.c b/src/util/virerror.c
> index f198f27957..5f26d59777 100644
> --- a/src/util/virerror.c
> +++ b/src/util/virerror.c
> 
> 
> ...

Sorry, hope I'll be more careful in the future.

Nikolay

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list