On 10/06/2015 05:01 PM, John Snow wrote:
>> + ret = load_bitmap(bs_file, dirty_bitmap_table,
>> bmh->dirty_bitmap_table_size, bitmap);
>> + if (ret < 0) {
>> + error_setg_errno(errp, -ret, "Could not read bitmap from image");
>> + goto finish;
>> + }
>> +
>> +finish:
>> + if (*errp != NULL) {
>> + bdrv_release_dirty_bitmap(bs_for, bitmap);
>> + bitmap = NULL;
>> + }
>> + g_free(dirty_bitmap_table);
>
>
> I think we're not supposed to be reaching into errp to check its
> implementation detail like this ... the usual paradigm I see is just
> "goto fail" or similar statements instead of checking for
> error-or-success in a shared return block.
>
If you have to make a decision based on whether an error was detected,
then you MUST pass a local error (as your caller may have passed NULL
because they don't care if you fail, even though you care if your helper
fails). As in:
Error *err = NULL;
...
if (ret < 0) {
error_setg_errno(&err, -ret, "Could not read...");
}
finish:
if (err) {
bdrv_release_dirty_bitmap(...);
error_propagate(errp, err);
bitmap = NULL;
}
In short, any code that does *errp is a potential NULL dereference. The
comments in error.h help explain the paradigms.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
