Potential Null dereference

Potential Null dereference

[Mansour Ahmadi]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

Hi,

Nullness of  needs to be checked here:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221

pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...

While it is done at 2 other locations:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477

if (bs->backing == NULL) { return}

Thanks,
Mansour

[-- Attachment #2: Type: text/html, Size: 4761 bytes --]

Re: Potential Null dereference

[Philippe Mathieu-Daudé]

On 3/24/20 4:05 AM, Mansour Ahmadi wrote:
> Hi,
> 
> Nullness of  needs to be checked here:
> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221
> 
> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...
> 
> 
> While it is done at 2 other locations:
> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477
> 
> if(bs->backing== NULL) {return}

Thanks for spotting this. Do you mind sending a patch?

> 
> Thanks,
> Mansour
> 

Re: Potential Null dereference

[Kevin Wolf]

Am 24.03.2020 um 08:14 hat Philippe Mathieu-Daudé geschrieben:
> On 3/24/20 4:05 AM, Mansour Ahmadi wrote:
> > Hi,
> > 
> > Nullness of  needs to be checked here:
> > https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221
> > 
> > pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...

Do you have a reproducer? It's not obvious to me how bs->backing could
be NULL here.

> > 
> > While it is done at 2 other locations:
> > https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
> > https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477

Commit 18775ff3269 made the change for mirror, however its commit
message is terse and doesn't say anything about the scenario where it
would happen. We also didn't add a test case for it. I would have
expected that failure to add the backing file would immediately error
out and not try to refresh the filename first.

backup-top.c has the check from the beginning. I assume it just copied
it from mirror.

Vladimir, do you remember the details?

Kevin

Re: Potential Null dereference

[Vladimir Sementsov-Ogievskiy]

24.03.2020 12:50, Kevin Wolf wrote:
> Am 24.03.2020 um 08:14 hat Philippe Mathieu-Daudé geschrieben:
>> On 3/24/20 4:05 AM, Mansour Ahmadi wrote:
>>> Hi,
>>>
>>> Nullness of  needs to be checked here:
>>> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221
>>>
>>> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...
> 
> Do you have a reproducer? It's not obvious to me how bs->backing could
> be NULL here.
> 
>>>
>>> While it is done at 2 other locations:
>>> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
>>> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477
> 
> Commit 18775ff3269 made the change for mirror, however its commit
> message is terse and doesn't say anything about the scenario where it
> would happen. We also didn't add a test case for it. I would have
> expected that failure to add the backing file would immediately error
> out and not try to refresh the filename first.
> 
> backup-top.c has the check from the beginning. I assume it just copied
> it from mirror.
> 
> Vladimir, do you remember the details?
> 

No :(
But I believe that "Backing may be zero after failed bdrv_attach_child in bdrv_set_backing_hd, which leads to SIGSEGV." means real problem, probably reproduced on some experiment branch, not on master.

Let's try most simple check:

apply the following:

=============================
diff --git a/include/block/block.h b/include/block/block.h
index e569a4d747..dc20b55075 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -298,6 +298,8 @@ BdrvChild *bdrv_open_child(const char *filename,
                             const BdrvChildRole *child_role,
                             bool allow_none, Error **errp);
  BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+                            bool bdrv_attach_child_fail, Error **errp);
  void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
                           Error **errp);
  int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
diff --git a/block.c b/block.c
index a2542c977b..21b6124d73 100644
--- a/block.c
+++ b/block.c
@@ -2743,8 +2743,8 @@ static bool bdrv_inherits_from_recursive(BlockDriverState *child,
   * Sets the backing file link of a BDS. A new reference is created; callers
   * which don't need their own reference any more must call bdrv_unref().
   */
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
-                         Error **errp)
+void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
+                            bool bdrv_attach_child_fail, Error **errp)
  {
      bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
          bdrv_inherits_from_recursive(backing_hd, bs);
@@ -2766,8 +2766,14 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
          goto out;
      }

-    bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing,
-                                    errp);
+    if (bdrv_attach_child_fail) {
+        bs->backing = NULL;
+        error_setg(errp, "Unpredicted failure :)");
+    } else {
+        bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing,
+                                        errp);
+    }
+
      /* If backing_hd was already part of bs's backing chain, and
       * inherits_from pointed recursively to bs then let's update it to
       * point directly to bs (else it will become NULL). */
@@ -2779,6 +2785,12 @@ out:
      bdrv_refresh_limits(bs, NULL);
  }

+void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+                         Error **errp)
+{
+    bdrv_set_backing_hd_ex(bs, backing_hd, false, errp);
+}
  /*
   * Opens the backing file for a BlockDriverState if not yet open
   *
diff --git a/block/mirror.c b/block/mirror.c
index 447051dbc6..907bb2b718 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -675,7 +675,7 @@ static int mirror_exit_common(Job *job)
      if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
          BlockDriverState *backing = s->is_none_mode ? src : s->base;
          if (backing_bs(target_bs) != backing) {
-            bdrv_set_backing_hd(target_bs, backing, &local_err);
+            bdrv_set_backing_hd_ex(target_bs, backing, true, &local_err);
              if (local_err) {
                  error_report_err(local_err);
                  ret = -EPERM;
@@ -1477,6 +1477,7 @@ static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs)
      if (bs->backing == NULL) {
          /* we can be here after failed bdrv_attach_child in
           * bdrv_set_backing_hd */
+        abort();
          return;
      }
      pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),


==============================================

[root@kvm qemu-iotests]# git grep -il mirror ??? | xargs
030 041 044 094 109 127 129 132 139 141 151 152 155 156 185 194 218 219 229 235 248 255 257 281

check -qcow2 030 041 044 094 109 127 129 132 139 141 151 152 155 156 185 194 218 219 229 235 248 255 257 281
...
Failures: 041 141 155


Have several core dumps, look at one:


Wow! These crashes are due to another bug: use after free!

Fix it:

diff --git a/block/mirror.c b/block/mirror.c
index 907bb2b718..22cf48e46a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
              bdrv_set_backing_hd_ex(target_bs, backing, true, &local_err);
              if (local_err) {
                  error_report_err(local_err);
+                local_err = NULL;
                  ret = -EPERM;
              }
          }
@@ -716,6 +717,7 @@ static int mirror_exit_common(Job *job)
          bdrv_drained_end(target_bs);
          if (local_err) {
              error_report_err(local_err);
+            local_err = NULL;
              ret = -EPERM;
          }
      }


Roll:


make -j9 && check -qcow2 030 041 044 094 109 127 129 132 139 141 151 152 155 156 185 194 218 219 229 235 248 255 257 281

Aha, new crashes! Let's look at them.

41 and 155 failed with crash, 141 without but I see "+{"error": {"class": "GenericError", "desc": "Block device drv0 is in use"}}" in its output.

Hmm, but all crashes are because of assert(QTAILQ_EMPTY(&all_bdrv_states)); in bdrv_close_all..

So, we have a problem, but another one..

==============

Hm, looking through our downstream branches, I found that originally the commit
"block/mirror: check backing in bdrv_mirror_top_refresh_filename" was to fix crash in our internal test.

I'll provide all my investigation from our jira below, it was 28 Sep 2017


#0 bdrv_mirror_top_refresh_filename (bs=0x5616df9a7400, opts=0x5616df268400) at block/mirror.c:1203
#1 0x00005616ddc3d35f in bdrv_refresh_filename (bs=0x5616df9a7400) at block.c:4739
#2 0x00005616ddc3d672 in bdrv_set_backing_hd (bs=bs@entry=0x5616df9a7400, backing_hd=backing_hd@entry=0x5616df25c000, errp=errp@entry=0x7ffff7896a20) at block.c:2035
#3 0x00005616ddc3dee3 in bdrv_append (bs_new=bs_new@entry=0x5616df9a7400, bs_top=bs_top@entry=0x5616df25c000, errp=errp@entry=0x7ffff7896ad8) at block.c:3168
#4 0x00005616ddc84e5f in mirror_start_job (job_id=job_id@entry=0x5616ddd16a31 "commit", bs=bs@entry=0x5616df25c000, creation_flags=creation_flags@entry=0, target=target@entry=0x5616df262800,
replaces=replaces@entry=0x0, speed=speed@entry=0, granularity=65536, granularity@entry=0, buf_size=16777216, buf_size@entry=0, backing_mode=backing_mode@entry=MIRROR_LEAVE_BACKING_CHAIN,
on_source_error=on_source_error@entry=BLOCKDEV_ON_ERROR_REPORT, on_target_error=on_target_error@entry=BLOCKDEV_ON_ERROR_REPORT, unmap=unmap@entry=true, cb=cb@entry=0x5616ddc35470 <common_block_job_cb>,
opaque=opaque@entry=0x7ffff7896c80, errp=errp@entry=0x7ffff7896bd0, driver=driver@entry=0x5616ddf8d100 <commit_active_job_driver>, is_none_mode=is_none_mode@entry=false, base=base@entry=0x5616df262800,
auto_complete=auto_complete@entry=false, filter_node_name=filter_node_name@entry=0x0) at block/mirror.c:1314
#5 0x00005616ddc87580 in commit_active_start (job_id=job_id@entry=0x5616ddd16a31 "commit", bs=bs@entry=0x5616df25c000, base=base@entry=0x5616df262800, creation_flags=creation_flags@entry=0,
speed=speed@entry=0, on_error=on_error@entry=BLOCKDEV_ON_ERROR_REPORT, filter_node_name=filter_node_name@entry=0x0, cb=cb@entry=0x5616ddc35470 <common_block_job_cb>, opaque=opaque@entry=0x7ffff7896c80,
errp=errp@entry=0x7ffff7896c78, auto_complete=auto_complete@entry=false) at block/mirror.c:1463
#6 0x00005616ddc33a68 in img_commit (argc=<optimized out>, argv=<optimized out>) at qemu-img.c:1013
#7 0x00005616ddc2fa79 in main (argc=4, argv=0x7ffff7896e00) at qemu-img.c:4548

--

static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts)
{ bdrv_refresh_filename(bs->backing->bs); pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->backing->bs->filename); }

(gdb) p bs->backing
$2 = (BdrvChild *) 0x0


--


(gdb) p errp
$3 = (Error **) 0x7ffff7896a20
(gdb) p *errp
$4 = (Error *) 0x5616df1c2660
(gdb) p **errp
$5 =
{msg = 0x5616df2554e0 "Failed to get \"write\" lock", err_class = ERROR_CLASS_GENERIC_ERROR, src = 0x5616ddd267fe "block/file-posix.c", func = 0x5616ddd26fe0 <__func__.27999> "raw_check_lock_bytes", line = 682, hint = 0x5616df1fe520}

void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
Error **errp)
{
if (backing_hd)
{ bdrv_ref(backing_hd); }

if (bs->backing)
{ bdrv_unref_child(bs, bs->backing); }

if (!backing_hd)
{ bs->backing = NULL; goto out; }

bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing,
errp);
if (!bs->backing)
{ bdrv_unref(backing_hd); }

bdrv_refresh_filename(bs);

(and we call refresh, when backing is zero because of an error)

out:
bdrv_refresh_limits(bs, NULL);
}

===========================================


So, we can see, that bdrv_refresh_filename() that led to crash is gone in commit f30c66ba6e417a0 "block: Use bdrv_refresh_filename() to pull", since 4.0...

Do we have the bug now? I can't say. Any thoughts?

-- 
Best regards,
Vladimir

Re: Potential Null dereference

[Vladimir Sementsov-Ogievskiy]

24.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote:
> 24.03.2020 12:50, Kevin Wolf wrote:
>> Am 24.03.2020 um 08:14 hat Philippe Mathieu-Daudé geschrieben:
>>> On 3/24/20 4:05 AM, Mansour Ahmadi wrote:
>>>> Hi,
>>>>
>>>> Nullness of  needs to be checked here:
>>>> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/commit.c#L221
>>>>
>>>> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),...
>>
>> Do you have a reproducer? It's not obvious to me how bs->backing could
>> be NULL here.
>>
>>>>
>>>> While it is done at 2 other locations:
>>>> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/backup-top.c#L113
>>>> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/block/mirror.c#L1477
>>
>> Commit 18775ff3269 made the change for mirror, however its commit
>> message is terse and doesn't say anything about the scenario where it
>> would happen. We also didn't add a test case for it. I would have
>> expected that failure to add the backing file would immediately error
>> out and not try to refresh the filename first.
>>
>> backup-top.c has the check from the beginning. I assume it just copied
>> it from mirror.
>>
>> Vladimir, do you remember the details?
>>
> 
> No :(
> But I believe that "Backing may be zero after failed bdrv_attach_child in bdrv_set_backing_hd, which leads to SIGSEGV." means real problem, probably reproduced on some experiment branch, not on master.
> 
> Let's try most simple check:
> 
> apply the following:
> 
> =============================
> diff --git a/include/block/block.h b/include/block/block.h
> index e569a4d747..dc20b55075 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -298,6 +298,8 @@ BdrvChild *bdrv_open_child(const char *filename,
>                              const BdrvChildRole *child_role,
>                              bool allow_none, Error **errp);
>   BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
> +void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
> +                            bool bdrv_attach_child_fail, Error **errp);
>   void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
>                            Error **errp);
>   int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
> diff --git a/block.c b/block.c
> index a2542c977b..21b6124d73 100644
> --- a/block.c
> +++ b/block.c
> @@ -2743,8 +2743,8 @@ static bool bdrv_inherits_from_recursive(BlockDriverState *child,
>    * Sets the backing file link of a BDS. A new reference is created; callers
>    * which don't need their own reference any more must call bdrv_unref().
>    */
> -void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> -                         Error **errp)
> +void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
> +                            bool bdrv_attach_child_fail, Error **errp)
>   {
>       bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
>           bdrv_inherits_from_recursive(backing_hd, bs);
> @@ -2766,8 +2766,14 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
>           goto out;
>       }
> 
> -    bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing,
> -                                    errp);
> +    if (bdrv_attach_child_fail) {
> +        bs->backing = NULL;
> +        error_setg(errp, "Unpredicted failure :)");
> +    } else {
> +        bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing,
> +                                        errp);
> +    }
> +
>       /* If backing_hd was already part of bs's backing chain, and
>        * inherits_from pointed recursively to bs then let's update it to
>        * point directly to bs (else it will become NULL). */
> @@ -2779,6 +2785,12 @@ out:
>       bdrv_refresh_limits(bs, NULL);
>   }
> 
> +void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> +                         Error **errp)
> +{
> +    bdrv_set_backing_hd_ex(bs, backing_hd, false, errp);
> +}
>   /*
>    * Opens the backing file for a BlockDriverState if not yet open
>    *
> diff --git a/block/mirror.c b/block/mirror.c
> index 447051dbc6..907bb2b718 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -675,7 +675,7 @@ static int mirror_exit_common(Job *job)
>       if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>           BlockDriverState *backing = s->is_none_mode ? src : s->base;
>           if (backing_bs(target_bs) != backing) {
> -            bdrv_set_backing_hd(target_bs, backing, &local_err);
> +            bdrv_set_backing_hd_ex(target_bs, backing, true, &local_err);
>               if (local_err) {
>                   error_report_err(local_err);
>                   ret = -EPERM;
> @@ -1477,6 +1477,7 @@ static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs)
>       if (bs->backing == NULL) {
>           /* we can be here after failed bdrv_attach_child in
>            * bdrv_set_backing_hd */
> +        abort();
>           return;
>       }
>       pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
> 
> 
> ==============================================
> 
> [root@kvm qemu-iotests]# git grep -il mirror ??? | xargs
> 030 041 044 094 109 127 129 132 139 141 151 152 155 156 185 194 218 219 229 235 248 255 257 281
> 
> check -qcow2 030 041 044 094 109 127 129 132 139 141 151 152 155 156 185 194 218 219 229 235 248 255 257 281
> ...
> Failures: 041 141 155
> 
> 
> Have several core dumps, look at one:
> 
> 
> Wow! These crashes are due to another bug: use after free!
> 
> Fix it:
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 907bb2b718..22cf48e46a 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
>               bdrv_set_backing_hd_ex(target_bs, backing, true, &local_err);
>               if (local_err) {
>                   error_report_err(local_err);
> +                local_err = NULL;
>                   ret = -EPERM;
>               }
>           }
> @@ -716,6 +717,7 @@ static int mirror_exit_common(Job *job)
>           bdrv_drained_end(target_bs);
>           if (local_err) {
>               error_report_err(local_err);
> +            local_err = NULL;
>               ret = -EPERM;
>           }
>       }
> 
> 
> Roll:
> 
> 
> make -j9 && check -qcow2 030 041 044 094 109 127 129 132 139 141 151 152 155 156 185 194 218 219 229 235 248 255 257 281
> 
> Aha, new crashes! Let's look at them.
> 
> 41 and 155 failed with crash, 141 without but I see "+{"error": {"class": "GenericError", "desc": "Block device drv0 is in use"}}" in its output.
> 
> Hmm, but all crashes are because of assert(QTAILQ_EMPTY(&all_bdrv_states)); in bdrv_close_all..
> 
> So, we have a problem, but another one..

Investigate it a bit more.

Accordingly to comment, bdrv_attach_child unrefs child bs even on failure, so let's do

--- a/block.c
+++ b/block.c
@@ -2768,6 +2768,7 @@ void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,

      if (bdrv_attach_child_fail) {
          bs->backing = NULL;
+        bdrv_unref(backing_hd);
          error_setg(errp, "Unpredicted failure :)");
      } else {
          bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing,


- then, all three tests fails, but without crashes. OK.

The only interesting thing is that, it seems that bdrv_attach_child doesn't unref child on all failure paths.

It calls bdrv_root_attach_child..

which doesn't unref child on the path

if (bdrv_get_aio_context(child_bs) != ctx) {
   ...
    if (ret < 0) {
          error_propagate(errp, local_err);
          g_free(child);
          bdrv_abort_perm_update(child_bs);
          return NULL;
    }
}

or am I wrong?



-- 
Best regards,
Vladimir

Re: Potential Null dereference

[Kevin Wolf]

Am 24.03.2020 um 13:37 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 24.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote:
> > Aha, new crashes! Let's look at them.
> > 
> > 41 and 155 failed with crash, 141 without but I see "+{"error": {"class": "GenericError", "desc": "Block device drv0 is in use"}}" in its output.
> > 
> > Hmm, but all crashes are because of assert(QTAILQ_EMPTY(&all_bdrv_states)); in bdrv_close_all..
> > 
> > So, we have a problem, but another one..
> 
> Investigate it a bit more.
> 
> Accordingly to comment, bdrv_attach_child unrefs child bs even on failure, so let's do
> 
> --- a/block.c
> +++ b/block.c
> @@ -2768,6 +2768,7 @@ void bdrv_set_backing_hd_ex(BlockDriverState *bs, BlockDriverState *backing_hd,
> 
>      if (bdrv_attach_child_fail) {
>          bs->backing = NULL;
> +        bdrv_unref(backing_hd);
>          error_setg(errp, "Unpredicted failure :)");
>      } else {
>          bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing,
> 
> 
> - then, all three tests fails, but without crashes. OK.
> 
> The only interesting thing is that, it seems that bdrv_attach_child doesn't unref child on all failure paths.
> 
> It calls bdrv_root_attach_child..
> 
> which doesn't unref child on the path
> 
> if (bdrv_get_aio_context(child_bs) != ctx) {
>   ...
>    if (ret < 0) {
>          error_propagate(errp, local_err);
>          g_free(child);
>          bdrv_abort_perm_update(child_bs);
>          return NULL;
>    }
> }
> 
> or am I wrong?

I think you're right, we need a bdrv_unref() there. The function comment
makes it clear that bdrv_root_attach_child() is supposed to consume the
reference both in success and error cases.

Kevin

Re: potential null dereference

[René Scharfe]

Am 15.12.2009 13:41, schrieb Jiri Slaby:
> Hi,
> 
> Stanse found the following error in unpack-trees.c:
> dereferencing NULL pointer here.[. * o src_index]
> 
> int unpack_trees(unsigned len, struct tree_desc *t, struct
> unpack_trees_options *o)
> {
>  int ret;
>  static struct cache_entry *dfc;
> ...
>  if (o->src_index) {                   <-- loc0
>   o->result.timestamp.sec = o->src_index->timestamp.sec;
>   o->result.timestamp.nsec = o->src_index->timestamp.nsec;
>  }
>  o->merge_size = len;
> 
>  if (!dfc)
>   dfc = xcalloc(1, ((1 + (0) + 8) & ~7));
>  o->df_conflict_entry = dfc;
> 
>  if (len) {
> ...
>  }
> 
>  if (o->merge) {
>   while (o->pos < o->src_index->cache_nr) { <-- here
> 
> It triggers, because there is a test for o->src_index being NULL at
> loc0, but here, it is dereferenced without a check. Can this happen
> (e.g. does o->merge != NULL imply o->src_index != NULL)?

Running "git grep -w -B70 unpack_trees" and looking for "src_index"
using less' search command showed me that src_index is never NULL when
unpack_trees() is called.

> Further, there is a warning in log-tree.c:
> pointer always points to valid memory here, but checking for not
> NULL.[parents]
> 
> static int log_tree_diff(struct rev_info *opt, struct commit *commit,
> struct log_info *log)
> {
>  int showed_log;
>  struct commit_list *parents;
>  unsigned const char *sha1 = commit->object.sha1;
> 
>  if (!opt->diff && !((&opt->diffopt)->flags & (1 << 14)))
>   return 0;
> 
> 
>  parents = commit->parents;
>  if (!parents) {            <-- loc0
>   if (opt->show_root_diff) {
>    diff_root_tree_sha1(sha1, "", &opt->diffopt);
>    log_tree_diff_flush(opt);
>   }
>   return !opt->loginfo;     <-- loc1
>  }
> 
>  if (parents && parents->next) { <-- here
> 
> I.e. if parents was NULL at loc0, we escaped at loc1. But we check
> parents against NULL here again.

The check may be duplicate, but I suspect removing it won't change the
resulting object code -- the compiler should be smart enough to come to
the same conclusion.

Thanks,
René

potential null dereference

[Jiri Slaby]

Hi,

Stanse found the following error in unpack-trees.c:
dereferencing NULL pointer here.[. * o src_index]

int unpack_trees(unsigned len, struct tree_desc *t, struct
unpack_trees_options *o)
{
 int ret;
 static struct cache_entry *dfc;
...
 if (o->src_index) {                   <-- loc0
  o->result.timestamp.sec = o->src_index->timestamp.sec;
  o->result.timestamp.nsec = o->src_index->timestamp.nsec;
 }
 o->merge_size = len;

 if (!dfc)
  dfc = xcalloc(1, ((1 + (0) + 8) & ~7));
 o->df_conflict_entry = dfc;

 if (len) {
...
 }

 if (o->merge) {
  while (o->pos < o->src_index->cache_nr) { <-- here

It triggers, because there is a test for o->src_index being NULL at
loc0, but here, it is dereferenced without a check. Can this happen
(e.g. does o->merge != NULL imply o->src_index != NULL)?






Further, there is a warning in log-tree.c:
pointer always points to valid memory here, but checking for not
NULL.[parents]

static int log_tree_diff(struct rev_info *opt, struct commit *commit,
struct log_info *log)
{
 int showed_log;
 struct commit_list *parents;
 unsigned const char *sha1 = commit->object.sha1;

 if (!opt->diff && !((&opt->diffopt)->flags & (1 << 14)))
  return 0;


 parents = commit->parents;
 if (!parents) {            <-- loc0
  if (opt->show_root_diff) {
   diff_root_tree_sha1(sha1, "", &opt->diffopt);
   log_tree_diff_flush(opt);
  }
  return !opt->loginfo;     <-- loc1
 }

 if (parents && parents->next) { <-- here

I.e. if parents was NULL at loc0, we escaped at loc1. But we check
parents against NULL here again.

thanks,
-- 
js