Re: [libvirt] PATCH: 15/28: Reduce return points for storage driver
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Tue, Dec 02, 2008 at 03:02:17PM +0100, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> > This patch reduces the number of return points in the storage driver
> > methods
> ...
> > diff --git a/src/storage_driver.c b/src/storage_driver.c
> ...
> > @@ -893,7 +924,7 @@ storagePoolListVolumes(virStoragePoolPtr
> >
> > cleanup:
> > for (n = 0 ; n < maxnames ; n++)
> > - VIR_FREE(names[i]);
> > + VIR_FREE(names[n]);
> >
> > memset(names, 0, maxnames);
> > return -1;
>
> This might be worth putting in a separate bug-fix patch.
> At first I thought this was fixing a serious bug,
> but you can see that i is always smaller than maxnames,
> so the fix is just plugging a leak.
>
> However, in looking at this I spotted a real problem:
> There are numerous statements like this:
>
> memset(names, 0, maxnames);
>
> That zeros out only 1/4 or 1/8 of the memory it should.
> It should be doing this:
>
> memset(names, 0, maxnames * sizeof (*names));
memset() is horribly error prone - also have the classic of getting
the 2 & 3rd args the wrong way around. How about adding to memory.h
something like
#define VIR_ZERO(buf, nelement) \
memset(buf, 0, sizeof(*(buf)) * nelement)
Sure, I'll do that after your 28-part change goes in.
But how about a more generic name like MEM_ZERO?
#define MEM_ZERO(buf, nelement) \
memset((buf), 0, sizeof(*(buf)) * (nelement))
also using your xalloc_oversized magic ?
If you want to automatically handle oversized memset, we'd have
to make it a function and change all users to test the return value.
That'd be pretty invasive. With malloc/realloc/calloc, where
everyone was already testing the return value, it was easy.
But testing memset's return value isn't useful, so no one does that.
An alternative is to abort on detection of an oversized count*size.
That's not library friendly at all, but better than memory corruption
or other run-time misbehavior.