installboot: double free

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

installboot: double free

Michael Mikonos
Hello,

In installboot's fileprefix() function r is the return value
of realpath(). If snprintf() fails free(r) happens twice---
the second time is at label "err". From what I see the behavior
was introduced in util.c revision 1.12.
Does this fix look OK?

- Michael


Index: util.c
===================================================================
RCS file: /cvs/src/usr.sbin/installboot/util.c,v
retrieving revision 1.12
diff -u -p -r1.12 util.c
--- util.c 3 Jul 2018 20:14:41 -0000 1.12
+++ util.c 6 Nov 2018 08:26:45 -0000
@@ -125,6 +125,7 @@ fileprefix(const char *base, const char
  }
  n = snprintf(s, PATH_MAX, "%s/%s", r, b);
  free(r);
+ r = NULL;
  if (n < 1 || n >= PATH_MAX) {
  warn("snprintf");
  goto err;

Reply | Threaded
Open this post in threaded view
|

Re: installboot: double free

Otto Moerbeek
On Tue, Nov 06, 2018 at 04:35:05PM +0800, Michael Mikonos wrote:

> Hello,
>
> In installboot's fileprefix() function r is the return value
> of realpath(). If snprintf() fails free(r) happens twice---
> the second time is at label "err". From what I see the behavior
> was introduced in util.c revision 1.12.
> Does this fix look OK?

Yes, but I perfer to move the free call to just before the no-error return.

        -Otto

>
> - Michael
>
>
> Index: util.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/installboot/util.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 util.c
> --- util.c 3 Jul 2018 20:14:41 -0000 1.12
> +++ util.c 6 Nov 2018 08:26:45 -0000
> @@ -125,6 +125,7 @@ fileprefix(const char *base, const char
>   }
>   n = snprintf(s, PATH_MAX, "%s/%s", r, b);
>   free(r);
> + r = NULL;
>   if (n < 1 || n >= PATH_MAX) {
>   warn("snprintf");
>   goto err;
>

Reply | Threaded
Open this post in threaded view
|

Re: installboot: double free

Michael Mikonos
On Tue, Nov 06, 2018 at 10:20:34AM +0100, Otto Moerbeek wrote:

> On Tue, Nov 06, 2018 at 04:35:05PM +0800, Michael Mikonos wrote:
>
> > Hello,
> >
> > In installboot's fileprefix() function r is the return value
> > of realpath(). If snprintf() fails free(r) happens twice---
> > the second time is at label "err". From what I see the behavior
> > was introduced in util.c revision 1.12.
> > Does this fix look OK?
>
> Yes, but I perfer to move the free call to just before the no-error return.
>

Sure, updated diff.

Index: util.c
===================================================================
RCS file: /cvs/src/usr.sbin/installboot/util.c,v
retrieving revision 1.12
diff -u -p -r1.12 util.c
--- util.c 3 Jul 2018 20:14:41 -0000 1.12
+++ util.c 6 Nov 2018 09:29:04 -0000
@@ -124,11 +124,11 @@ fileprefix(const char *base, const char
  goto err;
  }
  n = snprintf(s, PATH_MAX, "%s/%s", r, b);
- free(r);
  if (n < 1 || n >= PATH_MAX) {
  warn("snprintf");
  goto err;
  }
+ free(r);
  return (s);
 
 err:

Reply | Threaded
Open this post in threaded view
|

Re: installboot: double free

Otto Moerbeek
On Tue, Nov 06, 2018 at 05:32:47PM +0800, Michael Mikonos wrote:

> On Tue, Nov 06, 2018 at 10:20:34AM +0100, Otto Moerbeek wrote:
> > On Tue, Nov 06, 2018 at 04:35:05PM +0800, Michael Mikonos wrote:
> >
> > > Hello,
> > >
> > > In installboot's fileprefix() function r is the return value
> > > of realpath(). If snprintf() fails free(r) happens twice---
> > > the second time is at label "err". From what I see the behavior
> > > was introduced in util.c revision 1.12.
> > > Does this fix look OK?
> >
> > Yes, but I perfer to move the free call to just before the no-error return.
> >
>
> Sure, updated diff.

ok,

        -Otto

>
> Index: util.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/installboot/util.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 util.c
> --- util.c 3 Jul 2018 20:14:41 -0000 1.12
> +++ util.c 6 Nov 2018 09:29:04 -0000
> @@ -124,11 +124,11 @@ fileprefix(const char *base, const char
>   goto err;
>   }
>   n = snprintf(s, PATH_MAX, "%s/%s", r, b);
> - free(r);
>   if (n < 1 || n >= PATH_MAX) {
>   warn("snprintf");
>   goto err;
>   }
> + free(r);
>   return (s);
>  
>  err:
>