Philippe Mathieu-Daudé <f4...@amsat.org> writes:
> On 02/09/2018 12:24 PM, Paolo Bonzini wrote:
>> 1) string not null terminated in sysfs_find_group_file
>
> CID 1385854
>
>>
>> 2) NULL pointer dereference and dead local variable in nvme_init.
>
> CID 1385855
>
>>
>> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
>> ---
>> block/nvme.c | 14 ++++++--------
>> util/vfio-helpers.c | 2 +-
>> 2 files changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index e9d0e218fc..ce217ffc81 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -553,7 +553,6 @@ static int nvme_init(BlockDriverState *bs, const char
>> *device, int namespace,
>> uint64_t cap;
>> uint64_t timeout_ms;
>> uint64_t deadline, now;
>> - Error *local_err = NULL;
>>
>> qemu_co_mutex_init(&s->dma_map_lock);
>> qemu_co_queue_init(&s->dma_flush_queue);
>> @@ -645,11 +644,6 @@ static int nvme_init(BlockDriverState *bs, const char
>> *device, int namespace,
>> false, nvme_handle_event, nvme_poll_cb);
>>
>> nvme_identify(bs, namespace, errp);
>
> The problem seems local_err is not used as nvme_identify() argument;
> however this big function uses both errp and local_err so maybe clean it
> to keep one style is better.
>
> Isn't local_err + error_propagate() the cleaner way?
Cleaner no, actually correct, probably.
Passing errp directly is okay in certain circumstances. Quote
include/qapi/error.h:
* Receive an error and pass it on to the caller:
* Error *err = NULL;
* foo(arg, &err);
* if (err) {
* handle the error...
* error_propagate(errp, err);
* }
* where Error **errp is a parameter, by convention the last one.
*
* Do *not* "optimize" this to
* foo(arg, errp);
* if (*errp) { // WRONG!
* handle the error...
* }
* because errp may be NULL!
*
* But when all you do with the error is pass it on, please use
* foo(arg, errp);
* for readability.
However, ...
>> - if (local_err) {
>> - error_propagate(errp, local_err);
>> - ret = -EIO;
>> - goto fail_handler;
>> - }
... if nvme_identify() fails and sets an error, we now continue ...
>>
>> /* Set up command queues. */
>> if (!nvme_add_io_queue(bs, errp)) {
... to this call, where errp points to non-null. That's wrong, because
it'll crash when nvme_add_io_queue() tries to set an error.
include/qapi/error.h again:
* Receive and accumulate multiple errors (first one wins):
* Error *err = NULL, *local_err = NULL;
* foo(arg, &err);
* bar(arg, &local_err);
* error_propagate(&err, local_err);
* if (err) {
* handle the error...
* }
*
* Do *not* "optimize" this to
* foo(arg, &err);
* bar(arg, &err); // WRONG!
* if (err) {
* handle the error...
* }
* because this may pass a non-null err to bar().
I doubt you want to accumulate here.
[...]
