[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]

diff --git a/CHANGES b/CHANGES
index d6d9913315320bc318723a551032fec0ef1a548c..d31525f8e1ada7668a34a2038a93d1a3b464dc84 100644 (file)
--- 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 1642c2cdf84bb2cdef98539cd7678a9cd59fe6b9..3499692d9bc3e9560b72aeee4adb8db821a6a5db 100644 (file)
--- 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 bb8f05d52356b12f74d7eea8863bb4cb48f8245e..258aab453aca782bb977d580a4b46fb10ffa97d4 100644 (file)
--- 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.|
 |<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:|

diff --git a/bin/named/main.c b/bin/named/main.c
index 979f81b5562feef3709f70f942843a26a461783d..4fb056636dd29c3ccf021ddb272b1e916baaace9 100644 (file)
--- 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 ca5032c4340453b0198d4618faf56928ba1f8a2c..0c0b3c015d8294ab5e2a5cecce9cded8420ebc1b 100644 (file)
--- a/bin/named/named.docbook
+++ b/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
-            the only supported option is
+            Sets the default memory context options. If set to
             <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>

diff --git a/configure b/configure
index 1cc4fd2eca451c4f33e0cfecb78e58041bdc07c2..4e1ee1d5b8656fda836c27a85b82cd8426f88709 100755 (executable)
--- 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 bd745d14156f174abb4af8b09681ca8a05aecd82..2bbaf0098821fcfec8a2a3eb3b3233136c6fdac8 100644 (file)
--- 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 d6f31ac27f1d43f230987247345ee7046b1cbc4c..c111f755041041d215c719fcc55698b989f64c8a 100644 (file)
--- a/doc/arm/notes.xml
+++ b/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>

diff --git a/lib/isc/include/isc/mem.h b/lib/isc/include/isc/mem.h
index 2dd76208b069c56f90707528f6629eb06a821ab6..f1fa75841305abfa9cb532bcd5c5f602d5c59cb1 100644 (file)
--- 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 901545d552cf6aa25240774484d3609f7c87530d..5cd3c2aebcae582470c50fe88366ce6a4c71c6dc 100644 (file)
--- 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 4bfe17e976b20026286530d511d185986393afc2..a1d9cd2fa78074ea99069bfb53dc4c537fe62d79 100644 (file)
--- 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