From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mj6bQ-0004xn-5G for qemu-devel@nongnu.org; Thu, 03 Sep 2009 03:19:28 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mj6bK-0004tx-Nk for qemu-devel@nongnu.org; Thu, 03 Sep 2009 03:19:26 -0400 Received: from [199.232.76.173] (port=52088 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mj6bJ-0004td-Lk for qemu-devel@nongnu.org; Thu, 03 Sep 2009 03:19:22 -0400 Received: from mx20.gnu.org ([199.232.41.8]:14602) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Mj6bJ-0003CS-6X for qemu-devel@nongnu.org; Thu, 03 Sep 2009 03:19:21 -0400 Received: from mail-yx0-f201.google.com ([209.85.210.201]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mj6bI-0006BC-CV for qemu-devel@nongnu.org; Thu, 03 Sep 2009 03:19:20 -0400 Received: by yxe39 with SMTP id 39so761537yxe.18 for ; Thu, 03 Sep 2009 00:19:19 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1251937933-16778-1-git-send-email-jcd@tribudubois.net> References: <1251937933-16778-1-git-send-email-jcd@tribudubois.net> Date: Thu, 3 Sep 2009 10:19:19 +0300 Message-ID: Subject: Re: [Qemu-devel] [PATCH] Fix error checking and cleaning in path.c From: "Kirill A. Shutemov" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jean-Christophe DUBOIS Cc: qemu-devel@nongnu.org On Thu, Sep 3, 2009 at 3:32 AM, Jean-Christophe DUBOIS wrote: > Very little error checking was done in path.c and we were > leaking some memories in case of error. Nak. Do you really think you can do something useful after memory allocation fai= l? I think you can only do is call abort(). > > Signed-off-by: Jean-Christophe Dubois > --- > =C2=A0path.c | =C2=A0 76 ++++++++++++++++++++++++++++++++++++++++++++++++= +++++---------- > =C2=A01 files changed, 64 insertions(+), 12 deletions(-) > > diff --git a/path.c b/path.c > index cc9e007..24e8fdd 100644 > --- a/path.c > +++ b/path.c > @@ -40,15 +40,50 @@ static int strneq(const char *s1, unsigned int n, con= st char *s2) > > =C2=A0static struct pathelem *add_entry(struct pathelem *root, const char= *name); > > +static void free_entry(struct pathelem *ptr) > +{ > + =C2=A0 =C2=A0if(ptr) > + =C2=A0 =C2=A0{ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0while(ptr->num_entries) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ptr->entries[ptr->num_entr= ies -1]) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0free_entry(ptr->= entries[ptr->num_entries -1]); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_free(ptr->e= ntries[ptr->num_entries -1]); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ptr->entries[ptr= ->num_entries -1] =3D NULL; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ptr->num_entries--; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ptr->name) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_free(ptr->name); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ptr->name =3D NULL; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ptr->pathname) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0free(ptr->pathname); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ptr->pathname =3D NULL; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0} > + =C2=A0 =C2=A0} > +} > + > =C2=A0static struct pathelem *new_entry(const char *root, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct pathelem *parent, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *name) > =C2=A0{ > - =C2=A0 =C2=A0struct pathelem *new =3D malloc(sizeof(*new)); > - =C2=A0 =C2=A0new->name =3D strdup(name); > - =C2=A0 =C2=A0asprintf(&new->pathname, "%s/%s", root, name); > - =C2=A0 =C2=A0new->num_entries =3D 0; > + =C2=A0 =C2=A0struct pathelem *new =3D qemu_mallocz(sizeof(*new)); > + > + =C2=A0 =C2=A0if (new) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!(new->name =3D qemu_strdup(name))) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto fail; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (asprintf(&new->pathname, "%s/%s", root, = name) < 0) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto fail; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0new->num_entries =3D 0; > + =C2=A0 =C2=A0} > =C2=A0 =C2=A0 return new; > + > +fail: > + =C2=A0 =C2=A0free_entry(new); > + =C2=A0 =C2=A0qemu_free(new); > + =C2=A0 =C2=A0return NULL; > =C2=A0} > > =C2=A0#define streq(a,b) (strcmp((a), (b)) =3D=3D 0) > @@ -57,7 +92,7 @@ static struct pathelem *add_dir_maybe(struct pathelem *= path) > =C2=A0{ > =C2=A0 =C2=A0 DIR *dir; > > - =C2=A0 =C2=A0if ((dir =3D opendir(path->pathname)) !=3D NULL) { > + =C2=A0 =C2=A0if (path && ((dir =3D opendir(path->pathname)) !=3D NULL))= { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct dirent *dirent; > > =C2=A0 =C2=A0 =C2=A0 =C2=A0 while ((dirent =3D readdir(dir)) !=3D NULL) { > @@ -67,6 +102,7 @@ static struct pathelem *add_dir_maybe(struct pathelem = *path) > =C2=A0 =C2=A0 =C2=A0 =C2=A0 } > =C2=A0 =C2=A0 =C2=A0 =C2=A0 closedir(dir); > =C2=A0 =C2=A0 } > + > =C2=A0 =C2=A0 return path; > =C2=A0} > > @@ -74,13 +110,25 @@ static struct pathelem *add_entry(struct pathelem *r= oot, const char *name) > =C2=A0{ > =C2=A0 =C2=A0 root->num_entries++; > > - =C2=A0 =C2=A0root =3D realloc(root, sizeof(*root) > + =C2=A0 =C2=A0root =3D qemu_realloc(root, sizeof(*root) > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0+ si= zeof(root->entries[0])*root->num_entries); > > - =C2=A0 =C2=A0root->entries[root->num_entries-1] =3D new_entry(root->pat= hname, root, name); > - =C2=A0 =C2=A0root->entries[root->num_entries-1] > - =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D add_dir_maybe(root->entries[root->num_en= tries-1]); > + =C2=A0 =C2=A0if (root) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0root->entries[root->num_entries-1] > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=3D add_dir_maybe(new_entry(ro= ot->pathname, root, name)); > + > + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!root->entries[root->num_entries-1]) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0goto fail; > + =C2=A0 =C2=A0} else { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0/* if realloc failed, we have leaked some me= mory and we can't recover it */ > + =C2=A0 =C2=A0} > + > =C2=A0 =C2=A0 return root; > + > +fail: > + =C2=A0 =C2=A0free_entry(root); > + =C2=A0 =C2=A0qemu_free(root); > + =C2=A0 =C2=A0return NULL; > =C2=A0} > > =C2=A0/* This needs to be done after tree is stabilized (ie. no more real= locs!). */ > @@ -140,10 +188,14 @@ void init_paths(const char *prefix) > =C2=A0 =C2=A0 } else > =C2=A0 =C2=A0 =C2=A0 =C2=A0 pstrcpy(pref_buf, sizeof(pref_buf), prefix + = 1); > > - =C2=A0 =C2=A0base =3D new_entry("", NULL, pref_buf); > - =C2=A0 =C2=A0base =3D add_dir_maybe(base); > + =C2=A0 =C2=A0base =3D add_dir_maybe(new_entry("", NULL, pref_buf)); > + > + =C2=A0 =C2=A0if (!base) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0abort(); > + > =C2=A0 =C2=A0 if (base->num_entries =3D=3D 0) { > - =C2=A0 =C2=A0 =C2=A0 =C2=A0free (base); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0free_entry(base); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_free (base); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 base =3D NULL; > =C2=A0 =C2=A0 } else { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 set_parents(base, base); > -- > 1.6.0.4 > > > >