* [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt
@ 2019-11-27 18:30 Vladimir Sementsov-Ogievskiy
2019-11-29 14:03 ` Markus Armbruster
0 siblings, 1 reply; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-11-27 18:30 UTC (permalink / raw)
To: qemu-devel; +Cc: pburton, aleksandar.rikalo, vsementsov
fit_load_fdt forget to check that errp is not NULL and to zero it after
freeing. Fix it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
hw/core/loader-fit.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 953b16bc82..3ee9fb2f2e 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -200,7 +200,10 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
if (err == -ENOENT) {
load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
- error_free(*errp);
+ if (errp) {
+ error_free(*errp);
+ *errp = NULL;
+ }
} else if (err) {
error_prepend(errp, "unable to read FDT load address from FIT: ");
ret = err;
--
2.21.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt
2019-11-27 18:30 [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt Vladimir Sementsov-Ogievskiy
@ 2019-11-29 14:03 ` Markus Armbruster
2019-11-29 14:11 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 8+ messages in thread
From: Markus Armbruster @ 2019-11-29 14:03 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: pburton, aleksandar.rikalo, qemu-devel
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> fit_load_fdt forget to check that errp is not NULL and to zero it after
> freeing. Fix it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> hw/core/loader-fit.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> index 953b16bc82..3ee9fb2f2e 100644
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -200,7 +200,10 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
> err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
> if (err == -ENOENT) {
> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
> - error_free(*errp);
> + if (errp) {
> + error_free(*errp);
> + *errp = NULL;
> + }
> } else if (err) {
> error_prepend(errp, "unable to read FDT load address from FIT: ");
> ret = err;
This fixes latent bugs when fit_image_addr() fails with ENOENT:
* If a caller passes a null @errp, we crash dereferencing it
* Else, we pass a dangling Error * to the caller, and return success.
If a caller dereferences the Error * regardless, we have a
use-after-free.
Not fixed:
* If a caller passes &error_abort or &error_fatal, we die instead of
taking the recovery path.
We need to use a local_err here.
I'll search for the pattern, and post fix(es).
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt
2019-11-29 14:03 ` Markus Armbruster
@ 2019-11-29 14:11 ` Vladimir Sementsov-Ogievskiy
2019-11-29 14:12 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-11-29 14:11 UTC (permalink / raw)
To: Markus Armbruster; +Cc: pburton, aleksandar.rikalo, qemu-devel
29.11.2019 17:03, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> fit_load_fdt forget to check that errp is not NULL and to zero it after
>> freeing. Fix it.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>> hw/core/loader-fit.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>> index 953b16bc82..3ee9fb2f2e 100644
>> --- a/hw/core/loader-fit.c
>> +++ b/hw/core/loader-fit.c
>> @@ -200,7 +200,10 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>> err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>> if (err == -ENOENT) {
>> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>> - error_free(*errp);
>> + if (errp) {
>> + error_free(*errp);
>> + *errp = NULL;
>> + }
>> } else if (err) {
>> error_prepend(errp, "unable to read FDT load address from FIT: ");
>> ret = err;
>
> This fixes latent bugs when fit_image_addr() fails with ENOENT:
>
> * If a caller passes a null @errp, we crash dereferencing it
>
> * Else, we pass a dangling Error * to the caller, and return success.
> If a caller dereferences the Error * regardless, we have a
> use-after-free.
>
> Not fixed:
>
> * If a caller passes &error_abort or &error_fatal, we die instead of
> taking the recovery path.
No, if it is error_abort or error_fatal, we will not reach this point, the execution
finished before, when error was set.
>
> We need to use a local_err here.
>
> I'll search for the pattern, and post fix(es).
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt
2019-11-29 14:11 ` Vladimir Sementsov-Ogievskiy
@ 2019-11-29 14:12 ` Vladimir Sementsov-Ogievskiy
2019-11-29 15:23 ` Markus Armbruster
0 siblings, 1 reply; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-11-29 14:12 UTC (permalink / raw)
To: Markus Armbruster; +Cc: pburton, aleksandar.rikalo, qemu-devel
29.11.2019 17:11, Vladimir Sementsov-Ogievskiy wrote:
> 29.11.2019 17:03, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> fit_load_fdt forget to check that errp is not NULL and to zero it after
>>> freeing. Fix it.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>> hw/core/loader-fit.c | 5 ++++-
>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>>> index 953b16bc82..3ee9fb2f2e 100644
>>> --- a/hw/core/loader-fit.c
>>> +++ b/hw/core/loader-fit.c
>>> @@ -200,7 +200,10 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>> err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>>> if (err == -ENOENT) {
>>> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>> - error_free(*errp);
>>> + if (errp) {
>>> + error_free(*errp);
>>> + *errp = NULL;
>>> + }
>>> } else if (err) {
>>> error_prepend(errp, "unable to read FDT load address from FIT: ");
>>> ret = err;
>>
>> This fixes latent bugs when fit_image_addr() fails with ENOENT:
>>
>> * If a caller passes a null @errp, we crash dereferencing it
>>
>> * Else, we pass a dangling Error * to the caller, and return success.
>> If a caller dereferences the Error * regardless, we have a
>> use-after-free.
>>
>> Not fixed:
>>
>> * If a caller passes &error_abort or &error_fatal, we die instead of
>> taking the recovery path.
>
> No, if it is error_abort or error_fatal, we will not reach this point, the execution
> finished before, when error was set.
Ah, that is what you mention.. Hmm. Is it bad? At least crashing on
error_abort without any recovery should not be bad..
>
>>
>> We need to use a local_err here.
>>
>> I'll search for the pattern, and post fix(es).
>>
>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt
2019-11-29 14:12 ` Vladimir Sementsov-Ogievskiy
@ 2019-11-29 15:23 ` Markus Armbruster
2019-11-29 15:32 ` Vladimir Sementsov-Ogievskiy
2019-11-30 19:48 ` Markus Armbruster
0 siblings, 2 replies; 8+ messages in thread
From: Markus Armbruster @ 2019-11-29 15:23 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: pburton, aleksandar.rikalo, qemu-devel
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 29.11.2019 17:11, Vladimir Sementsov-Ogievskiy wrote:
>> 29.11.2019 17:03, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> fit_load_fdt forget to check that errp is not NULL and to zero it after
>>>> freeing. Fix it.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>> ---
>>>> hw/core/loader-fit.c | 5 ++++-
>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>>>> index 953b16bc82..3ee9fb2f2e 100644
>>>> --- a/hw/core/loader-fit.c
>>>> +++ b/hw/core/loader-fit.c
>>>> @@ -200,7 +200,10 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>> err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>>>> if (err == -ENOENT) {
>>>> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>>> - error_free(*errp);
>>>> + if (errp) {
>>>> + error_free(*errp);
>>>> + *errp = NULL;
>>>> + }
>>>> } else if (err) {
>>>> error_prepend(errp, "unable to read FDT load address from FIT: ");
>>>> ret = err;
>>>
>>> This fixes latent bugs when fit_image_addr() fails with ENOENT:
>>>
>>> * If a caller passes a null @errp, we crash dereferencing it
>>>
>>> * Else, we pass a dangling Error * to the caller, and return success.
>>> If a caller dereferences the Error * regardless, we have a
>>> use-after-free.
>>>
>>> Not fixed:
>>>
>>> * If a caller passes &error_abort or &error_fatal, we die instead of
>>> taking the recovery path.
>>
>> No, if it is error_abort or error_fatal, we will not reach this point, the execution
>> finished before, when error was set.
>
> Ah, that is what you mention.. Hmm. Is it bad? At least crashing on
> error_abort without any recovery should not be bad..
Latent bugs aren't bad, but they could possibly become bad :)
When you pass &err to fit_load_fdt(), where @err is a local variable,
the ENOENT case is not actually an error; fit_load_fdt() recovers fine
and succeeds.
When you pass &error_fatal or &error_abort, it should likewise not be an
error.
General rule: when you want to handle some (or all) of a function's
errors yourself, you must not pass your own @errp argument. Instead,
pass &err and propagate the errors you don't handle yourself with
error_propagate(errp, err).
>>> We need to use a local_err here.
>>>
>>> I'll search for the pattern, and post fix(es).
Found several bugs already...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt
2019-11-29 15:23 ` Markus Armbruster
@ 2019-11-29 15:32 ` Vladimir Sementsov-Ogievskiy
2019-11-29 20:03 ` Markus Armbruster
2019-11-30 19:48 ` Markus Armbruster
1 sibling, 1 reply; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-11-29 15:32 UTC (permalink / raw)
To: Markus Armbruster; +Cc: pburton, aleksandar.rikalo, qemu-devel
29.11.2019 18:23, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> 29.11.2019 17:11, Vladimir Sementsov-Ogievskiy wrote:
>>> 29.11.2019 17:03, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> fit_load_fdt forget to check that errp is not NULL and to zero it after
>>>>> freeing. Fix it.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>> ---
>>>>> hw/core/loader-fit.c | 5 ++++-
>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>>>>> index 953b16bc82..3ee9fb2f2e 100644
>>>>> --- a/hw/core/loader-fit.c
>>>>> +++ b/hw/core/loader-fit.c
>>>>> @@ -200,7 +200,10 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>>> err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>>>>> if (err == -ENOENT) {
>>>>> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>>>> - error_free(*errp);
>>>>> + if (errp) {
>>>>> + error_free(*errp);
>>>>> + *errp = NULL;
>>>>> + }
>>>>> } else if (err) {
>>>>> error_prepend(errp, "unable to read FDT load address from FIT: ");
>>>>> ret = err;
>>>>
>>>> This fixes latent bugs when fit_image_addr() fails with ENOENT:
>>>>
>>>> * If a caller passes a null @errp, we crash dereferencing it
>>>>
>>>> * Else, we pass a dangling Error * to the caller, and return success.
>>>> If a caller dereferences the Error * regardless, we have a
>>>> use-after-free.
>>>>
>>>> Not fixed:
>>>>
>>>> * If a caller passes &error_abort or &error_fatal, we die instead of
>>>> taking the recovery path.
>>>
>>> No, if it is error_abort or error_fatal, we will not reach this point, the execution
>>> finished before, when error was set.
>>
>> Ah, that is what you mention.. Hmm. Is it bad? At least crashing on
>> error_abort without any recovery should not be bad..
>
> Latent bugs aren't bad, but they could possibly become bad :)
>
> When you pass &err to fit_load_fdt(), where @err is a local variable,
> the ENOENT case is not actually an error; fit_load_fdt() recovers fine
> and succeeds.
>
> When you pass &error_fatal or &error_abort, it should likewise not be an
> error.
Ah, yes, understand now.
Interesting, that auto-propageted errp will not catch this. It will only
help with error_fatal, but not with error_abort..
So, in this case we should wrap error_abort too. And it in every function that
uses error_free.
And this means, that in this case we can't make error_abort crash at point where
actual error occures. That is very sad.
>
> General rule: when you want to handle some (or all) of a function's
> errors yourself, you must not pass your own @errp argument. Instead,
> pass &err and propagate the errors you don't handle yourself with
> error_propagate(errp, err).
>
>>>> We need to use a local_err here.
>>>>
>>>> I'll search for the pattern, and post fix(es).
>
> Found several bugs already...
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt
2019-11-29 15:32 ` Vladimir Sementsov-Ogievskiy
@ 2019-11-29 20:03 ` Markus Armbruster
0 siblings, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2019-11-29 20:03 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: pburton, aleksandar.rikalo, qemu-devel
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 29.11.2019 18:23, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 29.11.2019 17:11, Vladimir Sementsov-Ogievskiy wrote:
>>>> 29.11.2019 17:03, Markus Armbruster wrote:
>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>>
>>>>>> fit_load_fdt forget to check that errp is not NULL and to zero it after
>>>>>> freeing. Fix it.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>> ---
>>>>>> hw/core/loader-fit.c | 5 ++++-
>>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>>>>>> index 953b16bc82..3ee9fb2f2e 100644
>>>>>> --- a/hw/core/loader-fit.c
>>>>>> +++ b/hw/core/loader-fit.c
>>>>>> @@ -200,7 +200,10 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>>>> err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>>>>>> if (err == -ENOENT) {
>>>>>> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>>>>> - error_free(*errp);
>>>>>> + if (errp) {
>>>>>> + error_free(*errp);
>>>>>> + *errp = NULL;
>>>>>> + }
>>>>>> } else if (err) {
>>>>>> error_prepend(errp, "unable to read FDT load address from FIT: ");
>>>>>> ret = err;
>>>>>
>>>>> This fixes latent bugs when fit_image_addr() fails with ENOENT:
>>>>>
>>>>> * If a caller passes a null @errp, we crash dereferencing it
>>>>>
>>>>> * Else, we pass a dangling Error * to the caller, and return success.
>>>>> If a caller dereferences the Error * regardless, we have a
>>>>> use-after-free.
>>>>>
>>>>> Not fixed:
>>>>>
>>>>> * If a caller passes &error_abort or &error_fatal, we die instead of
>>>>> taking the recovery path.
>>>>
>>>> No, if it is error_abort or error_fatal, we will not reach this point, the execution
>>>> finished before, when error was set.
>>>
>>> Ah, that is what you mention.. Hmm. Is it bad? At least crashing on
>>> error_abort without any recovery should not be bad..
>>
>> Latent bugs aren't bad, but they could possibly become bad :)
>>
>> When you pass &err to fit_load_fdt(), where @err is a local variable,
>> the ENOENT case is not actually an error; fit_load_fdt() recovers fine
>> and succeeds.
>>
>> When you pass &error_fatal or &error_abort, it should likewise not be an
>> error.
>
> Ah, yes, understand now.
>
> Interesting, that auto-propageted errp will not catch this. It will only
> help with error_fatal, but not with error_abort..
>
> So, in this case we should wrap error_abort too. And it in every function that
> uses error_free.
>
> And this means, that in this case we can't make error_abort crash at point where
> actual error occures. That is very sad.
If your patches improve &error_abort's backtrace except for the (few)
functions calling error_free(), then I'd call the situation "a bit sad"
at most :)
[...]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt
2019-11-29 15:23 ` Markus Armbruster
2019-11-29 15:32 ` Vladimir Sementsov-Ogievskiy
@ 2019-11-30 19:48 ` Markus Armbruster
1 sibling, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2019-11-30 19:48 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: pburton, aleksandar.rikalo, qemu-devel
Markus Armbruster <armbru@redhat.com> writes:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> 29.11.2019 17:11, Vladimir Sementsov-Ogievskiy wrote:
>>> 29.11.2019 17:03, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> fit_load_fdt forget to check that errp is not NULL and to zero it after
>>>>> freeing. Fix it.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>> ---
>>>>> hw/core/loader-fit.c | 5 ++++-
>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>>>>> index 953b16bc82..3ee9fb2f2e 100644
>>>>> --- a/hw/core/loader-fit.c
>>>>> +++ b/hw/core/loader-fit.c
>>>>> @@ -200,7 +200,10 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>>> err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>>>>> if (err == -ENOENT) {
>>>>> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>>>> - error_free(*errp);
>>>>> + if (errp) {
>>>>> + error_free(*errp);
>>>>> + *errp = NULL;
>>>>> + }
>>>>> } else if (err) {
>>>>> error_prepend(errp, "unable to read FDT load address from FIT: ");
>>>>> ret = err;
>>>>
>>>> This fixes latent bugs when fit_image_addr() fails with ENOENT:
>>>>
>>>> * If a caller passes a null @errp, we crash dereferencing it
>>>>
>>>> * Else, we pass a dangling Error * to the caller, and return success.
>>>> If a caller dereferences the Error * regardless, we have a
>>>> use-after-free.
>>>>
>>>> Not fixed:
>>>>
>>>> * If a caller passes &error_abort or &error_fatal, we die instead of
>>>> taking the recovery path.
>>>
>>> No, if it is error_abort or error_fatal, we will not reach this point, the execution
>>> finished before, when error was set.
>>
>> Ah, that is what you mention.. Hmm. Is it bad? At least crashing on
>> error_abort without any recovery should not be bad..
>
> Latent bugs aren't bad, but they could possibly become bad :)
>
> When you pass &err to fit_load_fdt(), where @err is a local variable,
> the ENOENT case is not actually an error; fit_load_fdt() recovers fine
> and succeeds.
>
> When you pass &error_fatal or &error_abort, it should likewise not be an
> error.
>
> General rule: when you want to handle some (or all) of a function's
> errors yourself, you must not pass your own @errp argument. Instead,
> pass &err and propagate the errors you don't handle yourself with
> error_propagate(errp, err).
>
>>>> We need to use a local_err here.
>>>>
>>>> I'll search for the pattern, and post fix(es).
>
> Found several bugs already...
[PATCH 00/21] Error handling fixes, may contain 4.2 material
Message-Id: <20191130194240.10517-1-armbru@redhat.com>
I hope it doesn't clash too badly with your work.
PATCH 10 fixes the one we discussed above.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-11-30 20:03 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-27 18:30 [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt Vladimir Sementsov-Ogievskiy
2019-11-29 14:03 ` Markus Armbruster
2019-11-29 14:11 ` Vladimir Sementsov-Ogievskiy
2019-11-29 14:12 ` Vladimir Sementsov-Ogievskiy
2019-11-29 15:23 ` Markus Armbruster
2019-11-29 15:32 ` Vladimir Sementsov-Ogievskiy
2019-11-29 20:03 ` Markus Armbruster
2019-11-30 19:48 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.