diff --git a/CHANGES b/CHANGES index d6d9913315..d31525f8e1 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,12 @@ +4768. [func] By default, memory is no longer filled with tag values + when it is allocated or freed; this improves + performance but makes debugging of certain memory + issues more difficult. "named -M fill" turns memory + filling back on. (Building "configure + --enable-developer", turns memory fill on by + default again; it can then be disabled with + "named -M nofill".) [RT #45123] + 4767. [func] Add a new function, isc_buffer_printf(), which can be used to append a formatted string to the used region of a buffer. [RT #46201] diff --git a/OPTIONS b/OPTIONS index 1642c2cdf8..3499692d9b 100644 --- a/OPTIONS +++ b/OPTIONS @@ -5,9 +5,9 @@ defined in configure. Some of these settings are: Setting Description - Don't ovewrite memory when allocating or freeing --DISC_MEM_FILL=0 it; this improves performance but makes - debugging more difficult. + Overwrite memory with tag values when allocating +-DISC_MEM_DEFAULTFILL=1 or freeing it; this impairs performance but + makes debugging of memory problems easier. Don't track memory allocations by file and line -DISC_MEM_TRACKLINES=0 number; this improves performance but makes debugging more difficult. diff --git a/OPTIONS.md b/OPTIONS.md index bb8f05d523..258aab453a 100644 --- a/OPTIONS.md +++ b/OPTIONS.md @@ -13,7 +13,7 @@ Some of these settings are: |Setting |Description | |-----------------------------------|----------------------------------------| -|`-DISC_MEM_FILL=0`|Don't ovewrite memory when allocating or freeing it; this improves performance but makes debugging more difficult.| +|`-DISC_MEM_DEFAULTFILL=1`|Overwrite memory with tag values when allocating or freeing it; this impairs performance but makes debugging of memory problems easier.| |`-DISC_MEM_TRACKLINES=0`|Don't track memory allocations by file and line number; this improves performance but makes debugging more difficult.| |`-DISC_FACILITY=LOG_LOCAL0`|Change the default syslog facility for `named`| |`-DNS_CLIENT_DROPPORT=0`|Disable dropping queries from particular well-known ports:| diff --git a/bin/named/main.c b/bin/named/main.c index 979f81b556..4fb056636d 100644 --- a/bin/named/main.c +++ b/bin/named/main.c @@ -390,14 +390,20 @@ parse_int(char *arg, const char *desc) { static struct flag_def { const char *name; unsigned int value; + isc_boolean_t negate; } mem_debug_flags[] = { - { "none", 0}, - { "trace", ISC_MEM_DEBUGTRACE }, - { "record", ISC_MEM_DEBUGRECORD }, - { "usage", ISC_MEM_DEBUGUSAGE }, - { "size", ISC_MEM_DEBUGSIZE }, - { "mctx", ISC_MEM_DEBUGCTX }, - { NULL, 0 } + { "none", 0, ISC_FALSE }, + { "trace", ISC_MEM_DEBUGTRACE, ISC_FALSE }, + { "record", ISC_MEM_DEBUGRECORD, ISC_FALSE }, + { "usage", ISC_MEM_DEBUGUSAGE, ISC_FALSE }, + { "size", ISC_MEM_DEBUGSIZE, ISC_FALSE }, + { "mctx", ISC_MEM_DEBUGCTX, ISC_FALSE }, + { NULL, 0, ISC_FALSE } +}, mem_context_flags[] = { + { "external", ISC_MEMFLAG_INTERNAL, ISC_TRUE }, + { "fill", ISC_MEMFLAG_FILL, ISC_FALSE }, + { "nofill", ISC_MEMFLAG_FILL, ISC_TRUE }, + { NULL, 0, ISC_FALSE } }; static void @@ -416,7 +422,10 @@ set_flags(const char *arg, struct flag_def *defs, unsigned int *ret) { memcmp(arg, def->name, arglen) == 0) { if (def->value == 0) clear = ISC_TRUE; - *ret |= def->value; + if (def->negate) + *ret &= ~(def->value); + else + *ret |= def->value; goto found; } } @@ -519,8 +528,8 @@ parse_command_line(int argc, char *argv[]) { named_g_logfile = isc_commandline_argument; break; case 'M': - if (strcmp(isc_commandline_argument, "external") == 0) - isc_mem_defaultflags = 0; + set_flags(isc_commandline_argument, mem_context_flags, + &isc_mem_defaultflags); break; case 'm': set_flags(isc_commandline_argument, mem_debug_flags, @@ -1382,6 +1391,15 @@ main(int argc, char *argv[]) { pk11_result_register(); #endif +#if !ISC_MEM_DEFAULTFILL + /* + * Update the default flags to remove ISC_MEMFLAG_FILL + * before we parse the command line. If disabled here, + * it can be turned back on with -M fill. + */ + isc_mem_defaultflags &= ~ISC_MEMFLAG_FILL; +#endif + parse_command_line(argc, argv); #ifdef ENABLE_AFL diff --git a/bin/named/named.docbook b/bin/named/named.docbook index ca5032c434..0c0b3c015d 100644 --- a/bin/named/named.docbook +++ b/bin/named/named.docbook @@ -212,11 +212,17 @@ -M option - Sets the default memory context options. Currently - the only supported option is + Sets the default memory context options. If set to external, - which causes the internal memory manager to be bypassed + this causes the internal memory manager to be bypassed in favor of system-provided memory allocation functions. + If set to fill, + blocks of memory will be filled with tag values when allocated + or freed, to assist debugging of memory problems. + (nofill + disables this behavior, and is the default unless + named has been compiled with developer + options.) diff --git a/configure b/configure index 1cc4fd2eca..4e1ee1d5b8 100755 --- a/configure +++ b/configure @@ -11466,7 +11466,7 @@ fi XTARGETS= case "$enable_developer" in yes) - STD_CDEFINES="$STD_CDEFINES -DISC_LIST_CHECKINIT=1" + STD_CDEFINES="$STD_CDEFINES -DISC_MEM_DEFAULTFILL=1 -DISC_LIST_CHECKINIT=1" test "${enable_fixed_rrset+set}" = set || enable_fixed_rrset=yes test "${enable_querytrace+set}" = set || enable_querytrace=yes test "${with_atf+set}" = set || with_atf=yes diff --git a/configure.in b/configure.in index bd745d1415..2bbaf00988 100644 --- a/configure.in +++ b/configure.in @@ -62,7 +62,7 @@ AC_ARG_ENABLE(developer, [ --enable-developer enable developer build setti XTARGETS= case "$enable_developer" in yes) - STD_CDEFINES="$STD_CDEFINES -DISC_LIST_CHECKINIT=1" + STD_CDEFINES="$STD_CDEFINES -DISC_MEM_DEFAULTFILL=1 -DISC_LIST_CHECKINIT=1" test "${enable_fixed_rrset+set}" = set || enable_fixed_rrset=yes test "${enable_querytrace+set}" = set || enable_querytrace=yes test "${with_atf+set}" = set || with_atf=yes diff --git a/doc/arm/notes.xml b/doc/arm/notes.xml index d6f31ac27f..c111f75504 100644 --- a/doc/arm/notes.xml +++ b/doc/arm/notes.xml @@ -129,6 +129,18 @@ case restoration, hashing, and buffers. + + + When built with default configure options, + named no longer fills memory with tag + values when allocating or freeing it. This improves performance, + but makes it more difficult to debug certain memory-related + errors. The default is reversed if building with developer + options. named -M fill or + named -M nofill will set the behavior + accordingly regardless of build options. + + diff --git a/lib/isc/include/isc/mem.h b/lib/isc/include/isc/mem.h index 2dd76208b0..f1fa758413 100644 --- a/lib/isc/include/isc/mem.h +++ b/lib/isc/include/isc/mem.h @@ -52,23 +52,6 @@ typedef void (*isc_memfree_t)(void *, void *); #define ISC_MEM_CHECKOVERRUN 1 #endif -/*% - * Define ISC_MEM_FILL=1 to fill each block of memory returned to the system - * with the byte string '0xbe'. This helps track down uninitialized pointers - * and the like. On freeing memory, the space is filled with '0xde' for - * the same reasons. - * - * If we are performing a Coverity static analysis then ISC_MEM_FILL - * can hide bugs that would otherwise discovered so force to zero. - */ -#ifdef __COVERITY__ -#undef ISC_MEM_FILL -#define ISC_MEM_FILL 0 -#endif -#ifndef ISC_MEM_FILL -#define ISC_MEM_FILL 1 -#endif - /*% * Define ISC_MEMPOOL_NAMES=1 to make memory pools store a symbolic * name so that the leaking pool can be more readily identified in @@ -142,10 +125,12 @@ LIBISC_EXTERNAL_DATA extern unsigned int isc_mem_defaultflags; */ #define ISC_MEMFLAG_NOLOCK 0x00000001 /* no lock is necessary */ #define ISC_MEMFLAG_INTERNAL 0x00000002 /* use internal malloc */ -#if ISC_MEM_USE_INTERNAL_MALLOC -#define ISC_MEMFLAG_DEFAULT ISC_MEMFLAG_INTERNAL -#else +#define ISC_MEMFLAG_FILL 0x00000004 /* fill with pattern after alloc and frees */ + +#if !ISC_MEM_USE_INTERNAL_MALLOC #define ISC_MEMFLAG_DEFAULT 0 +#else +#define ISC_MEMFLAG_DEFAULT ISC_MEMFLAG_INTERNAL|ISC_MEMFLAG_FILL #endif diff --git a/lib/isc/mem.c b/lib/isc/mem.c index 901545d552..5cd3c2aebc 100644 --- a/lib/isc/mem.c +++ b/lib/isc/mem.c @@ -657,8 +657,8 @@ mem_getunlocked(isc__mem_t *ctx, size_t size) { ctx->maxmalloced = ctx->malloced; /* * If we don't set new_size to size, then the - * ISC_MEM_FILL code might write over bytes we - * don't own. + * ISC_MEMFLAG_FILL code might write over bytes we don't + * own. */ new_size = size; goto done; @@ -691,16 +691,14 @@ mem_getunlocked(isc__mem_t *ctx, size_t size) { ctx->inuse += new_size; done: - -#if ISC_MEM_FILL - if (ret != NULL) + if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0) && + ISC_LIKELY(ret != NULL)) memset(ret, 0xbe, new_size); /* Mnemonic for "beef". */ -#endif return (ret); } -#if ISC_MEM_FILL && ISC_MEM_CHECKOVERRUN +#if ISC_MEM_CHECKOVERRUN static inline void check_overrun(void *mem, size_t size, size_t new_size) { unsigned char *cp; @@ -724,9 +722,9 @@ mem_putunlocked(isc__mem_t *ctx, void *mem, size_t size) { /* * memput() called on something beyond our upper limit. */ -#if ISC_MEM_FILL - memset(mem, 0xde, size); /* Mnemonic for "dead". */ -#endif + if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0)) + memset(mem, 0xde, size); /* Mnemonic for "dead". */ + (ctx->memfree)(ctx->arg, mem); INSIST(ctx->stats[ctx->max_size].gets != 0U); ctx->stats[ctx->max_size].gets--; @@ -736,12 +734,12 @@ mem_putunlocked(isc__mem_t *ctx, void *mem, size_t size) { return; } -#if ISC_MEM_FILL + if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0)) { #if ISC_MEM_CHECKOVERRUN - check_overrun(mem, size, new_size); -#endif - memset(mem, 0xde, new_size); /* Mnemonic for "dead". */ + check_overrun(mem, size, new_size); #endif + memset(mem, 0xde, new_size); /* Mnemonic for "dead". */ + } /* * The free list uses the "rounded-up" size "new_size". @@ -771,19 +769,18 @@ mem_get(isc__mem_t *ctx, size_t size) { #if ISC_MEM_CHECKOVERRUN size += 1; #endif - ret = (ctx->memalloc)(ctx->arg, size); if (ret == NULL) ctx->memalloc_failures++; -#if ISC_MEM_FILL - if (ret != NULL) - memset(ret, 0xbe, size); /* Mnemonic for "beef". */ -#else -# if ISC_MEM_CHECKOVERRUN - if (ret != NULL) - ret[size-1] = 0xbe; -# endif + if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0)) { + if (ISC_LIKELY(ret != NULL)) + memset(ret, 0xbe, size); /* Mnemonic for "beef". */ + } else { +#if ISC_MEM_CHECKOVERRUN + if (ISC_LIKELY(ret != NULL)) + ret[size-1] = 0xbe; + } #endif return (ret); @@ -799,9 +796,8 @@ mem_put(isc__mem_t *ctx, void *mem, size_t size) { INSIST(((unsigned char *)mem)[size] == 0xbe); size += 1; #endif -#if ISC_MEM_FILL - memset(mem, 0xde, size); /* Mnemonic for "dead". */ -#endif + if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0)) + memset(mem, 0xde, size); /* Mnemonic for "dead". */ (ctx->memfree)(ctx->arg, mem); } diff --git a/util/altbuild.sh b/util/altbuild.sh index 4bfe17e976..a1d9cd2fa7 100644 --- a/util/altbuild.sh +++ b/util/altbuild.sh @@ -66,7 +66,7 @@ cd $builddir || exit 1 # Test a libtool / separate object dir / threadless build. -CFLAGS="-g -DISC_CHECK_NONE -DISC_MEM_FILL=0 -DISC_LIST_CHECKINIT" \ +CFLAGS="-g -DISC_CHECK_NONE -DISC_LIST_CHECKINIT" \ sh $srcdir/bind-*/configure --with-libtool \ --disable-threads --with-openssl --prefix=$instdir gmake clean @@ -77,7 +77,7 @@ gmake install # works, then run it. cd $srcdir/bind-* || exit 1 -CFLAGS="-g -DISC_CHECK_NONE -DISC_MEM_FILL=0 -DISC_LIST_CHECKINIT" \ +CFLAGS="-g -DISC_CHECK_NONE -DISC_LIST_CHECKINIT" \ sh configure --with-libtool --disable-threads --prefix=$instdir make make install