OS-6841: register_mem_int leaks mrp and returns success when memp conflicts

Details

Issue Type:Bug
Priority:4 - Normal
Status:Open
Created at:2018-03-27T16:18:09.784Z
Updated at:2019-07-17T22:13:32.895Z

People

Created by:Former user
Reported by:Former user

Related Issues

Labels

bhyve

Description

In bhyve/mem.c:

   214  static int
   215  register_mem_int(struct mmio_rb_tree *rbt, struct mem_range *memp)
   216  {
   217          struct mmio_rb_range *entry, *mrp;
   218          int             err;
   219  
   220          err = 0;
   221  
   222          mrp = malloc(sizeof(struct mmio_rb_range));
   223          
   224          if (mrp != NULL) {
   225                  mrp->mr_param = *memp;
   226                  mrp->mr_base = memp->base;
   227                  mrp->mr_end = memp->base + memp->size - 1;
   228                  pthread_rwlock_wrlock(&mmio_rwlock);
   229                  if (mmio_rb_lookup(rbt, memp->base, &entry) != 0)
   230                          err = mmio_rb_add(rbt, mrp);
   231                  pthread_rwlock_unlock(&mmio_rwlock);
   232                  if (err)
   233                          free(mrp);
   234          } else
   235                  err = ENOMEM;
   236  
   237          return (err);
   238  }

If mmio_rb_lookup() returns 0, mrp is leaked. I think that the right thing to do is to free mrp and return EEXIST. This can be accomplished with the following, as mmio_rb_add() does a check that is equivalent to the one performed by mmio_rb_lookup().

diff --git a/usr/src/cmd/bhyve/mem.c b/usr/src/cmd/bhyve/mem.c
index 2a9f430..0a238a1 100644
--- a/usr/src/cmd/bhyve/mem.c
+++ b/usr/src/cmd/bhyve/mem.c
@@ -226,8 +226,7 @@ register_mem_int(struct mmio_rb_tree *rbt, struct mem_range *memp)
                mrp->mr_base = memp->base;
                mrp->mr_end = memp->base + memp->size - 1;
                pthread_rwlock_wrlock(&mmio_rwlock);
-               if (mmio_rb_lookup(rbt, memp->base, &entry) != 0)
-                       err = mmio_rb_add(rbt, mrp);
+               err = mmio_rb_add(rbt, mrp);
                pthread_rwlock_unlock(&mmio_rwlock);
                if (err)
                        free(mrp);

That being said, perhaps silently ignoring this was intentional. I doubt it, but I don't know enough about this space to say with any certainty.

Comments

Comment by Former user
Created at 2018-03-27T16:30:13.140Z

This was found while looking at OS-6836. Applying the patch causes a failed assertion during insertion (register_bar) rather than during removal (unregister_bar).