Re: [PATCH v3 1/2] lvm: add lvm cache logical volume handling

[Michael Chang]

On Fri, Mar 13, 2020 at 01:23:42PM +0100, Daniel Kiper wrote:
> On Wed, Mar 04, 2020 at 02:44:52PM +0800, Michael Chang wrote:

[snip]

> > @@ -243,6 +279,8 @@ grub_lvm_detect (grub_disk_t disk,
> >
> >    if (! vg)
> >      {
> > +      struct cache_lv *cache_lvs = NULL;
> > +
> >        /* First time we see this volume group. We've to create the
> >      whole volume group structure. */
> >        vg = grub_malloc (sizeof (*vg));
> > @@ -672,6 +710,101 @@ grub_lvm_detect (grub_disk_t disk,
> >                       seg->nodes[seg->node_count - 1].name = tmp;
> >                     }
> >                 }
> > +             else if (grub_memcmp (p, "cache\"",
> > +                              sizeof ("cache\"") - 1) == 0)
> > +               {
> > +                 struct cache_lv *cache = NULL;
> > +
> > +                 char *p2, *p3;
> > +                 grub_ssize_t sz;
> 
> Why not grub_size_t?

That declaration was used in similar parts throughout the file and I
think I was just doing copy-and-paste without thinking too much. 

Certainly grub_size_t looks better for me too. I will fix that and send
next version.

> 
> > +
> > +                 cache = grub_zalloc (sizeof (*cache));
> > +                 if (!cache)
> > +                   goto cache_lv_fail;
> > +                 cache->lv = grub_zalloc (sizeof (*cache->lv));
> > +                 if (!cache->lv)
> > +                   goto cache_lv_fail;
> > +                 grub_memcpy (cache->lv, lv, sizeof (*cache->lv));
> > +
[snip]
> > +
> > +               cache_lv_fail:
> > +                 if (cache)
> > +                   {
> > +                     grub_free (cache->origin);
> > +                     grub_free (cache->cache_pool);
> > +                     grub_free (cache->lv->fullname);
> 
> If "cache = grub_zalloc (sizeof (*cache));" fails above then
> here GRUB crashes due to NULL pointer dereference...
> 
> > +                     grub_free (cache->lv->idname);
> 
> ...this has to be fixed too...
> 
> > +                     grub_free (cache->lv->name);
> 
> Ditto...

Arh, same mistake again! I'll fix them in next version.

Thanks a lot for review.

Michael

> 
> > +                     grub_free (cache);
> > +                   }
> > +                 goto fail4;
> > +               }
> >               else
> >                 {
> >  #ifdef GRUB_UTIL
> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel