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 {