Date: Thu, 29 Apr 2010 08:19:33 -0700 From: Alfred Perlstein <alfred@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-current@freebsd.org Subject: Re: fixes for enhanced coredump Message-ID: <20100429151933.GI36233@elvis.mu.org> In-Reply-To: <201004290740.43355.jhb@freebsd.org> References: <20100428171840.GS35381@elvis.mu.org> <201004290740.43355.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
* John Baldwin <jhb@freebsd.org> [100429 05:46] wrote:
> On Wednesday 28 April 2010 1:18:40 pm Alfred Perlstein wrote:
> > I was recently working on the enhanced coredumps
> > internal to Juniper and realized that there were
> > some defects in the code I pushed (mostly due to
> > mismerge), can someone please review?
> >
> > 1) don't allocate hostname[] on the stack
> > 2) don't leak the temp buffer in imgact_elf_coredump.
>
> Doesn't this leak hostname? I don't see it being free'd anywhere.
Thank you, I missed bringing that line over from JUNOS.
-Alfred
>
> > thank you,
> > -Alfred
> >
> >
> > Index: kern/kern_sig.c
> > ===================================================================
> > --- kern/kern_sig.c (revision 207329)
> > +++ kern/kern_sig.c (working copy)
> > @@ -3004,8 +3004,9 @@
> > char *temp;
> > size_t i;
> > int indexpos;
> > - char hostname[MAXHOSTNAMELEN];
> > + char *hostname;
> >
> > + hostname = NULL;
> > format = corefilename;
> > temp = malloc(MAXPATHLEN, M_TEMP, M_NOWAIT | M_ZERO);
> > if (temp == NULL)
> > @@ -3021,6 +3022,19 @@
> > sbuf_putc(&sb, '%');
> > break;
> > case 'H': /* hostname */
> > + if (hostname == NULL) {
> > + hostname = malloc(MAXHOSTNAMELEN,
> > + M_TEMP, M_NOWAIT);
> > + if (hostname == NULL) {
> > + log(LOG_ERR,
> > + "pid %ld (%s), uid (%lu): "
> > + "unable to alloc memory "
> > + "for corefile hostname\n",
> > + (long)pid, name,
> > + (u_long)uid);
> > + goto nomem;
> > + }
> > + }
> > getcredhostname(td->td_ucred, hostname,
> > sizeof(hostname));
> > sbuf_printf(&sb, "%s", hostname);
> > @@ -3054,9 +3068,10 @@
> > }
> > #endif
> > if (sbuf_overflowed(&sb)) {
> > - sbuf_delete(&sb);
> > log(LOG_ERR, "pid %ld (%s), uid (%lu): corename is too "
> > "long\n", (long)pid, name, (u_long)uid);
> > +nomem:
> > + sbuf_delete(&sb);
> > free(temp, M_TEMP);
> > return (NULL);
> > }
> > Index: kern/imgact_elf.c
> > ===================================================================
> > --- kern/imgact_elf.c (revision 207329)
> > +++ kern/imgact_elf.c (working copy)
> > @@ -1088,8 +1088,10 @@
> > hdrsize = 0;
> > __elfN(puthdr)(td, (void *)NULL, &hdrsize, seginfo.count);
> >
> > - if (hdrsize + seginfo.size >= limit)
> > - return (EFAULT);
> > + if (hdrsize + seginfo.size >= limit) {
> > + error = EFAULT;
> > + goto done;
> > + }
> >
> > /*
> > * Allocate memory for building the header, fill it up,
> > @@ -1097,7 +1099,8 @@
> > */
> > hdr = malloc(hdrsize, M_TEMP, M_WAITOK);
> > if (hdr == NULL) {
> > - return (EINVAL);
> > + error = EINVAL;
> > + goto done;
> > }
> > error = __elfN(corehdr)(td, vp, cred, seginfo.count, hdr, hdrsize,
> > gzfile);
> > @@ -1125,8 +1128,8 @@
> > curproc->p_comm, error);
> > }
> >
> > +done:
> > #ifdef COMPRESS_USER_CORES
> > -done:
> > if (core_buf)
> > free(core_buf, M_TEMP);
> > if (gzfile)
> > --
> > - Alfred Perlstein
> > .- AMA, VMOA #5191, 03 vmax, 92 gs500, 85 ch250, 07 zx10
> > .- FreeBSD committer
> > _______________________________________________
> > freebsd-current@freebsd.org mailing list
> > http://lists.freebsd.org/mailman/listinfo/freebsd-current
> > To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"
> >
>
> --
> John Baldwin
--
- Alfred Perlstein
.- AMA, VMOA #5191, 03 vmax, 92 gs500, 85 ch250, 07 zx10
.- FreeBSD committer
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100429151933.GI36233>