[master] turn off memory fill by default

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]

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]

OPTIONS

@@ -5,9 +5,9 @@ defined in configure.
 Some of these settings are:
 
 Setting                   Description
-                          Don't ovewrite memory when allocating or freeing
+                          Overwrite memory with tag values when allocating
--DISC_MEM_FILL=0          it; this improves performance but makes
+-DISC_MEM_DEFAULTFILL=1   or freeing it; this impairs performance but
-                          debugging more difficult.
+                          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.

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.|
 |<nobr>`-DISC_FACILITY=LOG_LOCAL0`</nobr>|Change the default syslog facility for `named`|
 |`-DNS_CLIENT_DROPPORT=0`|Disable dropping queries from particular well-known ports:|

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},
+	{ "none", 0, ISC_FALSE },
-	{ "trace",  ISC_MEM_DEBUGTRACE },
+	{ "trace",  ISC_MEM_DEBUGTRACE, ISC_FALSE },
-	{ "record", ISC_MEM_DEBUGRECORD },
+	{ "record", ISC_MEM_DEBUGRECORD, ISC_FALSE },
-	{ "usage", ISC_MEM_DEBUGUSAGE },
+	{ "usage", ISC_MEM_DEBUGUSAGE, ISC_FALSE },
-	{ "size", ISC_MEM_DEBUGSIZE },
+	{ "size", ISC_MEM_DEBUGSIZE, ISC_FALSE },
-	{ "mctx", ISC_MEM_DEBUGCTX },
+	{ "mctx", ISC_MEM_DEBUGCTX, ISC_FALSE },
-	{ NULL, 0 }
+	{ 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)
+			set_flags(isc_commandline_argument, mem_context_flags,
-				isc_mem_defaultflags = 0;
+				  &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

bin/named/named.docbook

@@ -212,11 +212,17 @@
         <term>-M <replaceable class="parameter">option</replaceable></term>
         <listitem>
           <para>
-            Sets the default memory context options.  Currently
+            Sets the default memory context options. If set to
-            the only supported option is
             <replaceable class="parameter">external</replaceable>,
-            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 <replaceable class="parameter">fill</replaceable>,
+            blocks of memory will be filled with tag values when allocated
+            or freed, to assist debugging of memory problems.
+            (<replaceable class="parameter">nofill</replaceable>
+            disables this behavior, and is the default unless
+            <command>named</command> has been compiled with developer
+            options.)
           </para>
         </listitem>
       </varlistentry>

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

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

doc/arm/notes.xml

@@ -129,6 +129,18 @@
 	      case restoration, hashing, and buffers.
 	    </para>
 	  </listitem>
+	  <listitem>
+	    <para>
+	      When built with default <command>configure</command> options,
+	      <command>named</command> 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. <command>named -M fill</command> or
+	      <command>named -M nofill</command> will set the behavior
+	      accordingly regardless of build options.
+	    </para>
+	  </listitem>
 	</itemizedlist>
       </listitem>
       <listitem>

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_FILL	0x00000004	 /* fill with pattern after alloc and frees */
-#define ISC_MEMFLAG_DEFAULT 	ISC_MEMFLAG_INTERNAL
+
-#else
+#if !ISC_MEM_USE_INTERNAL_MALLOC
 #define ISC_MEMFLAG_DEFAULT 	0
+#else
+#define ISC_MEMFLAG_DEFAULT	ISC_MEMFLAG_INTERNAL|ISC_MEMFLAG_FILL
 #endif
 
 

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
+		 * ISC_MEMFLAG_FILL code might write over bytes we don't
-		 * don't own.
+		 * 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_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0) &&
-#if ISC_MEM_FILL
+	    ISC_LIKELY(ret != NULL))
-	if (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
+		if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0))
-		memset(mem, 0xde, size); /* Mnemonic for "dead". */
+			memset(mem, 0xde, size); /* Mnemonic for "dead". */
-#endif
+
 		(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);
+		check_overrun(mem, size, new_size);
-#endif
-	memset(mem, 0xde, new_size); /* Mnemonic for "dead". */
 #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 (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0)) {
-	if (ret != NULL)
+		if (ISC_LIKELY(ret != NULL))
-		memset(ret, 0xbe, size); /* Mnemonic for "beef". */
+			memset(ret, 0xbe, size); /* Mnemonic for "beef". */
-#else
+	} else {
-#  if ISC_MEM_CHECKOVERRUN
+#if ISC_MEM_CHECKOVERRUN
-	if (ret != NULL)
+		if (ISC_LIKELY(ret != NULL))
-		ret[size-1] = 0xbe;
+			ret[size-1] = 0xbe;
-#  endif
+	}
 #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
+	if (ISC_UNLIKELY((ctx->flags & ISC_MEMFLAG_FILL) != 0))
-	memset(mem, 0xde, size); /* Mnemonic for "dead". */
+		memset(mem, 0xde, size); /* Mnemonic for "dead". */
-#endif
 	(ctx->memfree)(ctx->arg, mem);
 }
 

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