Bug#318879: udpkg: Fix minor memory leak
found 318879 udpkg/1.11
thanks
On Fri, Jul 29, 2005 at 07:32:32PM -0400, Joey Hess wrote:
> Petter Reinholdtsen wrote:
> > I believe there is a minor memory leak in udpkg. This patch fixes it.
> > It will need a review and some testing before it is commited.
> >
> > Index: status.c
> > ===================================================================
> > --- status.c (revisjon 29314)
> > +++ status.c (arbeidskopi)
> > @@ -106,6 +111,8 @@
> > strcat(multiple_lines, " ");
> > strcat(multiple_lines, buf);
> > }
> > + if (NULL != *ml)
> > + free(*ml);
> > *ml = multiple_lines;
> > ungetc(ch, f);
> > return EXIT_SUCCESS;
>
> Hmm, if you look at the callers of read_block(), all of them pass a
> pointer to strdup("") in which is silly if we'll always free it. So we
> could instead just:
>
> Index: status.c
> ===================================================================
> --- status.c (revision 29522)
> +++ status.c (working copy)
> @@ -143,7 +143,6 @@
> else if (strstr(buf, "Description: ") == buf)
> {
> p->description = strdup(buf+13);
> - p->long_description = strdup("");
> read_block(f, &p->long_description);
> }
> #ifdef DOL18N
> @@ -158,7 +157,6 @@
> buf[14] = '\0';
> l->language = strdup(buf+12);
> l->description = strdup(buf+16);
> - l->long_description = strdup("");
> read_block(f, &l->long_description);
>
> }
> @@ -198,7 +196,6 @@
> }
> else if (strstr(buf, "Conffiles: ") == buf)
> {
> - p->conffiles = strdup("");
> read_block(f, &p->conffiles);
> }
>
This caused udpkg to segfault (on Ubuntu, but I see no reason why it
should be Ubuntu-specific). Here's the start of read_block:
static int read_block(FILE *f, char **ml)
{
char ch;
char *multiple_lines = *ml;
char buf[BUFSIZE];
while (((ch = fgetc(f)) == ' ') && !feof(f)) {
fgets(buf, BUFSIZE, f);
multiple_lines = (char *) di_realloc(multiple_lines, strlen(multiple_lines) + strlen(buf) + 2);
So leaving *ml undefined on entry means that (a) realloc's behaviour is
undefined and (b) strlen's behaviour is undefined. Whoops.
Petter's patch would crash too. Since the pointer in *ml on entry may
have been realloced, attempting to free it may be equivalent to a
double-free, which could corrupt the malloc arena.
On the one hand, this is a tiny memory leak in udpkg, and it really
isn't worth a great deal of effort, so I would be inclined to just
revert this. On the other hand, if you think about it, read_block's
interface is pretty poor. It returns int but there is only one possible
return value, and it takes a char ** which is only ever initialised in
one way. This isn't good design. Here's a patch implementing an
improved interface, which is simpler, safer, and more efficient:
Index: debian/changelog
===================================================================
--- debian/changelog (revision 65318)
+++ debian/changelog (working copy)
@@ -1,3 +1,10 @@
+udpkg (1.12) UNRELEASED; urgency=low
+
+ * Redesign read_block interface, fixing crashes caused by memory leak fix
+ (closes: #318879).
+
+ -- Colin Watson <cjwatson@debian.org> Thu, 04 Nov 2010 17:12:53 +0000
+
udpkg (1.11) unstable; urgency=low
* Team upload
Index: status.c
===================================================================
--- status.c (revision 65318)
+++ status.c (working copy)
@@ -93,22 +93,26 @@
return buf;
}
-static int read_block(FILE *f, char **ml)
+static char *read_block(FILE *f)
{
char ch;
- char *multiple_lines = *ml;
+ char *multiple_lines = strdup("");
+ size_t len = 0;
char buf[BUFSIZE];
while (((ch = fgetc(f)) == ' ') && !feof(f)) {
+ size_t buflen;
+
fgets(buf, BUFSIZE, f);
- multiple_lines = (char *) di_realloc(multiple_lines, strlen(multiple_lines) + strlen(buf) + 2);
- memset(multiple_lines + strlen(multiple_lines), '\0', strlen(buf) + 2);
+ buflen = strlen(buf);
+ multiple_lines = (char *) di_realloc(multiple_lines, len + buflen + 2);
+ memset(multiple_lines + len, '\0', buflen + 2);
strcat(multiple_lines, " ");
strcat(multiple_lines, buf);
+ len += buflen + 1;
}
- *ml = multiple_lines;
ungetc(ch, f);
- return EXIT_SUCCESS;
+ return multiple_lines;
}
/*
@@ -143,7 +147,7 @@
else if (strstr(buf, "Description: ") == buf)
{
p->description = strdup(buf+13);
- read_block(f, &p->long_description);
+ p->long_description = read_block(f);
}
#ifdef SUPPORTL10N
else if (strstr(buf, "description-") == buf)
@@ -157,7 +161,7 @@
buf[14] = '\0';
l->language = strdup(buf+12);
l->description = strdup(buf+16);
- read_block(f, &l->long_description);
+ l->long_description = read_block(f);
}
#endif
/* This is specific to the Debian Installer. Ifdef? */
@@ -195,7 +199,7 @@
}
else if (strcasestr(buf, "Conffiles: ") == buf)
{
- read_block(f, &p->conffiles);
+ p->conffiles = read_block(f);
}
}
Since I've tested this and it works, I'm going to go ahead and upload
it.
--
Colin Watson [cjwatson@ubuntu.com]
Reply to: