[10/21] hw/core: Fix latent fit_load_fdt() error handling bug
diff mbox series

Message ID 20191130194240.10517-11-armbru@redhat.com
State New
Headers show
Series
  • Error handling fixes, may contain 4.2 material
Related show

Commit Message

Markus Armbruster Nov. 30, 2019, 7:42 p.m. UTC
fit_load_fdt() recovers from fit_image_addr() failing with ENOENT.
Except it doesn't when its @errp argument is &error_fatal or
&error_abort, because it blindly passes @errp to fit_image_addr().
Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()".

The bug can't bite as no caller actually passes &error_fatal or
&error_abort.  Fix it anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/loader-fit.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Dec. 5, 2019, 4:23 p.m. UTC | #1
30.11.2019 22:42, Markus Armbruster wrote:
> fit_load_fdt() recovers from fit_image_addr() failing with ENOENT.
> Except it doesn't when its @errp argument is &error_fatal or
> &error_abort, because it blindly passes @errp to fit_image_addr().
> Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()".
> 
> The bug can't bite as no caller actually passes &error_fatal or
> &error_abort.  Fix it anyway.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Hmm, actually it makes my
"[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt"
unnecessary. If you want you may drop my 01 (as it covers less problems),
and in this case you may note in your cover letter, that
errp = NULL is broken here too (may be nobady pass it?),
and normal errp is handled wrong, as *errp doesn't set to NULL after
error_free(*errp) (still, the only caller rely on return value more than on
err, so the problem can't be triggered with current code)

> ---
>   hw/core/loader-fit.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> index 953b16bc82..c465921b8f 100644
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>                           int cfg, void *opaque, const void *match_data,
>                           hwaddr kernel_end, Error **errp)
>   {
> +    Error *err = NULL;
>       const char *name;
>       const void *data;
>       const void *load_data;
>       hwaddr load_addr;
> -    int img_off, err;
> +    int img_off;
>       size_t sz;
>       int ret;
>   
> @@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>           return -EINVAL;
>       }
>   
> -    err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
> -    if (err == -ENOENT) {
> +    ret = fit_image_addr(itb, img_off, "load", &load_addr, &err);
> +    if (ret == -ENOENT) {
>           load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
> -        error_free(*errp);
> -    } else if (err) {
> -        error_prepend(errp, "unable to read FDT load address from FIT: ");
> -        ret = err;
> +        error_free(err);
> +    } else if (ret) {
> +        error_propagate_prepend(errp, err,
> +                                "unable to read FDT load address from FIT: ");
>           goto out;
>       }
>   
> 

So much attention to that function :)

I'd also propose the following:

diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index c465921b8f..2c9efeef7a 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
  {
      Error *err = NULL;
      const char *name;
-    const void *data;
-    const void *load_data;
+    void *data;
+    void *load_data;
      hwaddr load_addr;
      int img_off;
      size_t sz;
@@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,

      ret = 0;
  out:
-    g_free((void *) data);
+    g_free(data);
      if (data != load_data) {
-        g_free((void *) load_data);
+        g_free(load_data);
      }
      return ret;
  }


Or, even better:

--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
  {
      Error *err = NULL;
      const char *name;
-    const void *data;
+    g_autofree void *data = NULL;
+    g_autofree void *fdt_filter_data = NULL;
      const void *load_data;
      hwaddr load_addr;
      int img_off;
@@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
      if (ret == -ENOENT) {
          load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
          error_free(err);
+        return 0;
      } else if (ret) {
          error_propagate_prepend(errp, err,
                                  "unable to read FDT load address from FIT: ");
-        goto out;
+        return ret;
      }

      if (ldr->fdt_filter) {
-        load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
+        load_data = fdt_filter_data =
+                ldr->fdt_filter(opaque, data, match_data, &load_addr);
      }

      load_addr = ldr->addr_to_phys(opaque, load_addr);
      sz = fdt_totalsize(load_data);
      rom_add_blob_fixed(name, load_data, sz, load_addr);

-    ret = 0;
-out:
-    g_free((void *) data);
-    if (data != load_data) {
-        g_free((void *) load_data);
-    }
-    return ret;
+    return 0;
  }
Markus Armbruster Dec. 6, 2019, 7:46 a.m. UTC | #2
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 30.11.2019 22:42, Markus Armbruster wrote:
>> fit_load_fdt() recovers from fit_image_addr() failing with ENOENT.
>> Except it doesn't when its @errp argument is &error_fatal or
>> &error_abort, because it blindly passes @errp to fit_image_addr().
>> Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()".
>> 
>> The bug can't bite as no caller actually passes &error_fatal or
>> &error_abort.  Fix it anyway.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Hmm, actually it makes my
> "[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt"
> unnecessary. If you want you may drop my 01 (as it covers less problems),

Yes.

> and in this case you may note in your cover letter, that
> errp = NULL is broken here too (may be nobady pass it?),

You're right, null @errp is wrong, too.

> and normal errp is handled wrong, as *errp doesn't set to NULL after
> error_free(*errp)

Yes, that's wrong, too.  fit_load_fdt() itself doesn't use *errp after
freeing it, but it sets a trap for its callers.  

>                   (still, the only caller rely on return value more than on
> err, so the problem can't be triggered with current code)

True.

New commit message (based on v2's):

    hw/core: Fix fit_load_fdt() error API violations

    fit_load_fdt() passes @errp to fit_image_addr(), then recovers from
    ENOENT failures.  Passing @errp is wrong, because it works only as
    long as @errp is neither @error_fatal nor @error_abort.  Error
    recovery dereferences @errp.  That's also wrong; see the big comment
    in error.h.  Error recovery can leave *errp pointing to a freed
    Error object.  Wrong, it must be null on success.  Messed up in
    commit 3eb99edb48 "loader-fit: Wean off error_printf()".

    No caller actually passes such values, or uses *errp on success.

    Fix anyway: splice in a local Error *err, and error_propagate().

    Signed-off-by: Markus Armbruster <armbru@redhat.com>

Okay?

>
>> ---
>>   hw/core/loader-fit.c | 15 ++++++++-------
>>   1 file changed, 8 insertions(+), 7 deletions(-)
>> 
>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>> index 953b16bc82..c465921b8f 100644
>> --- a/hw/core/loader-fit.c
>> +++ b/hw/core/loader-fit.c
>> @@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>                           int cfg, void *opaque, const void *match_data,
>>                           hwaddr kernel_end, Error **errp)
>>   {
>> +    Error *err = NULL;
>>       const char *name;
>>       const void *data;
>>       const void *load_data;
>>       hwaddr load_addr;
>> -    int img_off, err;
>> +    int img_off;
>>       size_t sz;
>>       int ret;
>>   
>> @@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>           return -EINVAL;
>>       }
>>   
>> -    err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>> -    if (err == -ENOENT) {
>> +    ret = fit_image_addr(itb, img_off, "load", &load_addr, &err);
>> +    if (ret == -ENOENT) {
>>           load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>> -        error_free(*errp);
>> -    } else if (err) {
>> -        error_prepend(errp, "unable to read FDT load address from FIT: ");
>> -        ret = err;
>> +        error_free(err);
>> +    } else if (ret) {
>> +        error_propagate_prepend(errp, err,
>> +                                "unable to read FDT load address from FIT: ");
>>           goto out;
>>       }
>>   
>> 
>
> So much attention to that function :)
>
> I'd also propose the following:
>
> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> index c465921b8f..2c9efeef7a 100644
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>   {
>       Error *err = NULL;
>       const char *name;
> -    const void *data;
> -    const void *load_data;
> +    void *data;
> +    void *load_data;
>       hwaddr load_addr;
>       int img_off;
>       size_t sz;
> @@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>
>       ret = 0;
>   out:
> -    g_free((void *) data);
> +    g_free(data);
>       if (data != load_data) {
> -        g_free((void *) load_data);
> +        g_free(load_data);
>       }
>       return ret;
>   }
>
>
> Or, even better:
>
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>   {
>       Error *err = NULL;
>       const char *name;
> -    const void *data;
> +    g_autofree void *data = NULL;
> +    g_autofree void *fdt_filter_data = NULL;
>       const void *load_data;
>       hwaddr load_addr;
>       int img_off;
> @@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>       if (ret == -ENOENT) {
>           load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>           error_free(err);
> +        return 0;
>       } else if (ret) {
>           error_propagate_prepend(errp, err,
>                                   "unable to read FDT load address from FIT: ");
> -        goto out;
> +        return ret;
>       }
>
>       if (ldr->fdt_filter) {
> -        load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
> +        load_data = fdt_filter_data =
> +                ldr->fdt_filter(opaque, data, match_data, &load_addr);
>       }
>
>       load_addr = ldr->addr_to_phys(opaque, load_addr);
>       sz = fdt_totalsize(load_data);
>       rom_add_blob_fixed(name, load_data, sz, load_addr);
>
> -    ret = 0;
> -out:
> -    g_free((void *) data);
> -    if (data != load_data) {
> -        g_free((void *) load_data);
> -    }
> -    return ret;
> +    return 0;
>   }

Looks like a sensible separate cleanup patch to me.  Care to post it?

Thanks!
Vladimir Sementsov-Ogievskiy Dec. 6, 2019, 10:53 a.m. UTC | #3
06.12.2019 10:46, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 30.11.2019 22:42, Markus Armbruster wrote:
>>> fit_load_fdt() recovers from fit_image_addr() failing with ENOENT.
>>> Except it doesn't when its @errp argument is &error_fatal or
>>> &error_abort, because it blindly passes @errp to fit_image_addr().
>>> Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()".
>>>
>>> The bug can't bite as no caller actually passes &error_fatal or
>>> &error_abort.  Fix it anyway.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> Hmm, actually it makes my
>> "[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt"
>> unnecessary. If you want you may drop my 01 (as it covers less problems),
> 
> Yes.
> 
>> and in this case you may note in your cover letter, that
>> errp = NULL is broken here too (may be nobady pass it?),
> 
> You're right, null @errp is wrong, too.
> 
>> and normal errp is handled wrong, as *errp doesn't set to NULL after
>> error_free(*errp)
> 
> Yes, that's wrong, too.  fit_load_fdt() itself doesn't use *errp after
> freeing it, but it sets a trap for its callers.
> 
>>                    (still, the only caller rely on return value more than on
>> err, so the problem can't be triggered with current code)
> 
> True.
> 
> New commit message (based on v2's):
> 
>      hw/core: Fix fit_load_fdt() error API violations
> 
>      fit_load_fdt() passes @errp to fit_image_addr(), then recovers from
>      ENOENT failures.  Passing @errp is wrong, because it works only as
>      long as @errp is neither @error_fatal nor @error_abort.  Error
>      recovery dereferences @errp.  That's also wrong; see the big comment
>      in error.h.  Error recovery can leave *errp pointing to a freed
>      Error object.  Wrong, it must be null on success.  Messed up in
>      commit 3eb99edb48 "loader-fit: Wean off error_printf()".
> 
>      No caller actually passes such values, or uses *errp on success.
> 
>      Fix anyway: splice in a local Error *err, and error_propagate().
> 
>      Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Okay?

Yes) also add
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


> 
>>
>>> ---
>>>    hw/core/loader-fit.c | 15 ++++++++-------
>>>    1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>>> index 953b16bc82..c465921b8f 100644
>>> --- a/hw/core/loader-fit.c
>>> +++ b/hw/core/loader-fit.c
>>> @@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>                            int cfg, void *opaque, const void *match_data,
>>>                            hwaddr kernel_end, Error **errp)
>>>    {
>>> +    Error *err = NULL;
>>>        const char *name;
>>>        const void *data;
>>>        const void *load_data;
>>>        hwaddr load_addr;
>>> -    int img_off, err;
>>> +    int img_off;
>>>        size_t sz;
>>>        int ret;
>>>    
>>> @@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>            return -EINVAL;
>>>        }
>>>    
>>> -    err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>>> -    if (err == -ENOENT) {
>>> +    ret = fit_image_addr(itb, img_off, "load", &load_addr, &err);
>>> +    if (ret == -ENOENT) {
>>>            load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>> -        error_free(*errp);
>>> -    } else if (err) {
>>> -        error_prepend(errp, "unable to read FDT load address from FIT: ");
>>> -        ret = err;
>>> +        error_free(err);
>>> +    } else if (ret) {
>>> +        error_propagate_prepend(errp, err,
>>> +                                "unable to read FDT load address from FIT: ");
>>>            goto out;
>>>        }
>>>    
>>>
>>
>> So much attention to that function :)
>>
>> I'd also propose the following:
>>
>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>> index c465921b8f..2c9efeef7a 100644
>> --- a/hw/core/loader-fit.c
>> +++ b/hw/core/loader-fit.c
>> @@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>    {
>>        Error *err = NULL;
>>        const char *name;
>> -    const void *data;
>> -    const void *load_data;
>> +    void *data;
>> +    void *load_data;
>>        hwaddr load_addr;
>>        int img_off;
>>        size_t sz;
>> @@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>
>>        ret = 0;
>>    out:
>> -    g_free((void *) data);
>> +    g_free(data);
>>        if (data != load_data) {
>> -        g_free((void *) load_data);
>> +        g_free(load_data);
>>        }
>>        return ret;
>>    }
>>
>>
>> Or, even better:
>>
>> --- a/hw/core/loader-fit.c
>> +++ b/hw/core/loader-fit.c
>> @@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>    {
>>        Error *err = NULL;
>>        const char *name;
>> -    const void *data;
>> +    g_autofree void *data = NULL;
>> +    g_autofree void *fdt_filter_data = NULL;
>>        const void *load_data;
>>        hwaddr load_addr;
>>        int img_off;
>> @@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>        if (ret == -ENOENT) {
>>            load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>            error_free(err);
>> +        return 0;
>>        } else if (ret) {
>>            error_propagate_prepend(errp, err,
>>                                    "unable to read FDT load address from FIT: ");
>> -        goto out;
>> +        return ret;
>>        }
>>
>>        if (ldr->fdt_filter) {
>> -        load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
>> +        load_data = fdt_filter_data =
>> +                ldr->fdt_filter(opaque, data, match_data, &load_addr);
>>        }
>>
>>        load_addr = ldr->addr_to_phys(opaque, load_addr);
>>        sz = fdt_totalsize(load_data);
>>        rom_add_blob_fixed(name, load_data, sz, load_addr);
>>
>> -    ret = 0;
>> -out:
>> -    g_free((void *) data);
>> -    if (data != load_data) {
>> -        g_free((void *) load_data);
>> -    }
>> -    return ret;
>> +    return 0;
>>    }
> 
> Looks like a sensible separate cleanup patch to me.  Care to post it?
> 

Yes, I'll send
Vladimir Sementsov-Ogievskiy Jan. 10, 2020, 8:06 p.m. UTC | #4
06.12.2019 13:53, Vladimir Sementsov-Ogievskiy wrote:
> 06.12.2019 10:46, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 30.11.2019 22:42, Markus Armbruster wrote:
>>>> fit_load_fdt() recovers from fit_image_addr() failing with ENOENT.
>>>> Except it doesn't when its @errp argument is &error_fatal or
>>>> &error_abort, because it blindly passes @errp to fit_image_addr().
>>>> Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()".
>>>>
>>>> The bug can't bite as no caller actually passes &error_fatal or
>>>> &error_abort.  Fix it anyway.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> Hmm, actually it makes my
>>> "[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt"
>>> unnecessary. If you want you may drop my 01 (as it covers less problems),
>>
>> Yes.
>>
>>> and in this case you may note in your cover letter, that
>>> errp = NULL is broken here too (may be nobady pass it?),
>>
>> You're right, null @errp is wrong, too.
>>
>>> and normal errp is handled wrong, as *errp doesn't set to NULL after
>>> error_free(*errp)
>>
>> Yes, that's wrong, too.  fit_load_fdt() itself doesn't use *errp after
>> freeing it, but it sets a trap for its callers.
>>
>>>                    (still, the only caller rely on return value more than on
>>> err, so the problem can't be triggered with current code)
>>
>> True.
>>
>> New commit message (based on v2's):
>>
>>      hw/core: Fix fit_load_fdt() error API violations
>>
>>      fit_load_fdt() passes @errp to fit_image_addr(), then recovers from
>>      ENOENT failures.  Passing @errp is wrong, because it works only as
>>      long as @errp is neither @error_fatal nor @error_abort.  Error
>>      recovery dereferences @errp.  That's also wrong; see the big comment
>>      in error.h.  Error recovery can leave *errp pointing to a freed
>>      Error object.  Wrong, it must be null on success.  Messed up in
>>      commit 3eb99edb48 "loader-fit: Wean off error_printf()".
>>
>>      No caller actually passes such values, or uses *errp on success.
>>
>>      Fix anyway: splice in a local Error *err, and error_propagate().
>>
>>      Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Okay?
> 
> Yes) also add
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 
>>
>>>
>>>> ---
>>>>    hw/core/loader-fit.c | 15 ++++++++-------
>>>>    1 file changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>>>> index 953b16bc82..c465921b8f 100644
>>>> --- a/hw/core/loader-fit.c
>>>> +++ b/hw/core/loader-fit.c
>>>> @@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>>                            int cfg, void *opaque, const void *match_data,
>>>>                            hwaddr kernel_end, Error **errp)
>>>>    {
>>>> +    Error *err = NULL;
>>>>        const char *name;
>>>>        const void *data;
>>>>        const void *load_data;
>>>>        hwaddr load_addr;
>>>> -    int img_off, err;
>>>> +    int img_off;
>>>>        size_t sz;
>>>>        int ret;
>>>> @@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>>            return -EINVAL;
>>>>        }
>>>> -    err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>>>> -    if (err == -ENOENT) {
>>>> +    ret = fit_image_addr(itb, img_off, "load", &load_addr, &err);
>>>> +    if (ret == -ENOENT) {
>>>>            load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>>> -        error_free(*errp);
>>>> -    } else if (err) {
>>>> -        error_prepend(errp, "unable to read FDT load address from FIT: ");
>>>> -        ret = err;
>>>> +        error_free(err);
>>>> +    } else if (ret) {
>>>> +        error_propagate_prepend(errp, err,
>>>> +                                "unable to read FDT load address from FIT: ");
>>>>            goto out;
>>>>        }
>>>>
>>>
>>> So much attention to that function :)
>>>
>>> I'd also propose the following:
>>>
>>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>>> index c465921b8f..2c9efeef7a 100644
>>> --- a/hw/core/loader-fit.c
>>> +++ b/hw/core/loader-fit.c
>>> @@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>    {
>>>        Error *err = NULL;
>>>        const char *name;
>>> -    const void *data;
>>> -    const void *load_data;
>>> +    void *data;
>>> +    void *load_data;
>>>        hwaddr load_addr;
>>>        int img_off;
>>>        size_t sz;
>>> @@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>
>>>        ret = 0;
>>>    out:
>>> -    g_free((void *) data);
>>> +    g_free(data);
>>>        if (data != load_data) {
>>> -        g_free((void *) load_data);
>>> +        g_free(load_data);
>>>        }
>>>        return ret;
>>>    }
>>>
>>>
>>> Or, even better:
>>>
>>> --- a/hw/core/loader-fit.c
>>> +++ b/hw/core/loader-fit.c
>>> @@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>    {
>>>        Error *err = NULL;
>>>        const char *name;
>>> -    const void *data;
>>> +    g_autofree void *data = NULL;
>>> +    g_autofree void *fdt_filter_data = NULL;
>>>        const void *load_data;
>>>        hwaddr load_addr;
>>>        int img_off;
>>> @@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>        if (ret == -ENOENT) {
>>>            load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>>            error_free(err);
>>> +        return 0;
>>>        } else if (ret) {
>>>            error_propagate_prepend(errp, err,
>>>                                    "unable to read FDT load address from FIT: ");
>>> -        goto out;
>>> +        return ret;
>>>        }
>>>
>>>        if (ldr->fdt_filter) {
>>> -        load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr);
>>> +        load_data = fdt_filter_data =
>>> +                ldr->fdt_filter(opaque, data, match_data, &load_addr);
>>>        }
>>>
>>>        load_addr = ldr->addr_to_phys(opaque, load_addr);
>>>        sz = fdt_totalsize(load_data);
>>>        rom_add_blob_fixed(name, load_data, sz, load_addr);
>>>
>>> -    ret = 0;
>>> -out:
>>> -    g_free((void *) data);
>>> -    if (data != load_data) {
>>> -        g_free((void *) load_data);
>>> -    }
>>> -    return ret;
>>> +    return 0;
>>>    }
>>
>> Looks like a sensible separate cleanup patch to me.  Care to post it?
>>
> 
> Yes, I'll send
> 


Hm, it doesn't compile, as fit_load_image_alloc return type is const pointer..
So, I just don't understand the logic of the code (for me it's strange to
free pointer, returned by function, returning const pointer)

And what are the reasons, to make return type of fit_load_image_alloc constant,
I don't see.. IMHO, there are no reasons.
Same thing with .fdt_filter ...

I don't want to dig in, so, sorry, I'll not send :)
Markus Armbruster Jan. 13, 2020, 1:01 p.m. UTC | #5
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 06.12.2019 13:53, Vladimir Sementsov-Ogievskiy wrote:
>> 06.12.2019 10:46, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
[...]
>>>> So much attention to that function :)
>>>>
>>>> I'd also propose the following:
[...]
>>>
>>> Looks like a sensible separate cleanup patch to me.  Care to post it?
>>>
>> 
>> Yes, I'll send
>> 
>
>
> Hm, it doesn't compile, as fit_load_image_alloc return type is const pointer..
> So, I just don't understand the logic of the code (for me it's strange to
> free pointer, returned by function, returning const pointer)
>
> And what are the reasons, to make return type of fit_load_image_alloc constant,
> I don't see.. IMHO, there are no reasons.
> Same thing with .fdt_filter ...
>
> I don't want to dig in, so, sorry, I'll not send :)

That's okay.

Patch
diff mbox series

diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 953b16bc82..c465921b8f 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -178,11 +178,12 @@  static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
                         int cfg, void *opaque, const void *match_data,
                         hwaddr kernel_end, Error **errp)
 {
+    Error *err = NULL;
     const char *name;
     const void *data;
     const void *load_data;
     hwaddr load_addr;
-    int img_off, err;
+    int img_off;
     size_t sz;
     int ret;
 
@@ -197,13 +198,13 @@  static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
         return -EINVAL;
     }
 
-    err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
-    if (err == -ENOENT) {
+    ret = fit_image_addr(itb, img_off, "load", &load_addr, &err);
+    if (ret == -ENOENT) {
         load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
-        error_free(*errp);
-    } else if (err) {
-        error_prepend(errp, "unable to read FDT load address from FIT: ");
-        ret = err;
+        error_free(err);
+    } else if (ret) {
+        error_propagate_prepend(errp, err,
+                                "unable to read FDT load address from FIT: ");
         goto out;
     }