On Tue, Oct 18, 2016 at 03:23:32AM +0200, Theo Buehler wrote:
> On Sun, Oct 16, 2016 at 01:14:35PM +0200, Otto Moerbeek wrote:
> > Hi,
> >
> > this diff is somewhat big since I decided to rewrite wrterror() to be
> > able to get better error messages.
> >
> > Please review and test this with malloc option C (and other flags). An
> > example run:
> >
> > a.out(85360) in free(): chunk canary corrupted 0x13c68f696000
> > 0x18a92@0x18a88
> >
> > This means I overwrote a byte at offset 0x18a92 in a chunk of size
> > 0x18a88 that is located at 0x13c68f696000.
> >
> > Only max 32 bytes after the requested size are filled with 0xdb and
> > checked on free.
>
> This is very nice and reads very well. I tested it with C and CJ and S
> on amd64 and it seems to work fine.
>
> A few small comments about the new wrterror():
>
> In
>
> + snprintf(pidbuf, sizeof(pidbuf), "%s(%d) in %s(): ", __progname,
> + getpid(), d->func ? d->func : "unknown");
>
> I think you should truncate an insanely long __progname to 50 characters
> to avoid overwriting the more important info that follows:
>
> snprintf(pidbuf, sizeof(pidbuf), "%.50s(%d) in %s(): ", __progname,
> getpid(), d->func ? d->func : "unknown");
>
> because the longest possible string after __progname is
>
> "(12345) in posix_memalign(): "
>
> which has 29 characters, so there is room for 50 non-NULs in pidbuf[80],
> and these should be plenty enough to identify the offending program.
indeed.
>
>
> I'm slightly concerned about running strlen on pidbuf[] and buf[] in
> case {,v}snprintf fail. Wouldn't it be cleaner to do something like
> this:
>
> if ((ret = snprintf(pidbuf, ...)) >= 0) {
> iov[0].iov_base = pidbuf;
> iov[0].iov_len = ret;
> } else {
> /* hardcoded message */
> }
>
That is certainly not correct: snprintf and friends return the length as
it would have been if an infinite buffer was passed in.
So the strlen should stay. I'll make a new diff soon though with the
error checking, although it might be overkill for this case.
-Otto
> and a small style nit:
>
> > @@ -1181,6 +1177,13 @@ omalloc(struct dir_info *pool, size_t sz
> > memset(p, SOME_JUNK,
> > psz - mopts.malloc_guard);
> > }
> > + else if (mopts.chunk_canaries) {
>
> } else if (mopts.chunk_canaries) {
>
> > + size_t csz = psz - mopts.malloc_guard - sz;
> > +
> > + if (csz > CHUNK_CHECK_LENGTH)
> > + csz = CHUNK_CHECK_LENGTH;
> > + memset(p + sz, SOME_JUNK, csz);
> > + }
> > }
> >
> > } else {
