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

[Michael Chang <mchang@suse.com>]

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
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel