Rework and make DEBUG macros consistent.

Remove unused -DCONF_DEBUG and -DBN_CTX_DEBUG.

Rename REF_PRINT to REF_DEBUG for consistency, and add a new
tracing category and use it for printing reference counts.

Rename -DDEBUG_UNUSED to -DUNUSED_RESULT_DEBUG

Fix BN_DEBUG_RAND so it compiles and, when set, force DEBUG_RAND to
be set also.

Rename engine_debug_ref to be ENGINE_REF_PRINT also for consistency.

Fixes #15357

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15353)

CHANGES.md

@@ -30,22 +30,30 @@ breaking changes, and mappings for the large list of deprecated functions.
 
 ### Changes between 1.1.1 and 3.0 [xx XXX xxxx]
 
+ * Rework and make DEBUG macros consistent. Remove unused -DCONF_DEBUG,
+   -DBN_CTX_DEBUG, and REF_PRINT. Add a new tracing category and use it for
+   printing reference counts. Rename -DDEBUG_UNUSED to -DUNUSED_RESULT_DEBUG
+   Fix BN_DEBUG_RAND so it compiles and, when set, force DEBUG_RAND to be set
+   also. Rename engine_debug_ref to be ENGINE_REF_PRINT also for consistency.
+
+   *Rich Salz*
+
  * The signatures of the functions to get and set options on SSL and
    SSL_CTX objects changed from "unsigned long" to "uint64_t" type.
    Some source code changes may be required.
 
-   * Rich Salz *
+   *Rich Salz*
 
  * Client-initiated renegotiation is disabled by default. To allow it, use
    the -client_renegotiation option, the SSL_OP_ALLOW_CLIENT_RENEGOTIATION
    flag, or the "ClientRenegotiation" config parameter as appropriate.
 
-   * Rich Salz *
+   *Rich Salz*
 
  * Add "abspath" and "includedir" pragma's to config files, to prevent,
    or modify relative pathname inclusion.
 
-   * Rich Salz *
+   *Rich Salz*
 
  * OpenSSL includes a cryptographic module that is intended to be FIPS 140-2
    validated. Please consult the README-FIPS and

Configurations/90-team.norelease.conf

@@ -12,14 +12,17 @@ my %targets = (
     "debug" => {
         inherit_from     => [ 'BASE_unix' ],
         cc               => "gcc",
-        cflags           => "-DBN_DEBUG -DREF_DEBUG -DCONF_DEBUG -DBN_CTX_DEBUG -DOPENSSL_NO_ASM -ggdb -g2 -Wformat -Wshadow -Wmissing-prototypes -Wmissing-declarations -Werror",
+        cflags           => combine(join(' ', @gcc_devteam_warn),
+                                "-DOPENSSL_NO_ASM -ggdb -g2"
+                              . " -DBN_DEBUG -DBN_RAND_DEBUG"
+                              ),
         thread_scheme    => "(unknown)",
     },
     "debug-erbridge" => {
         inherit_from     => [ 'BASE_unix', "x86_64_asm" ],
         cc               => "gcc",
         cflags           => combine(join(' ', @gcc_devteam_warn),
-                                    "-DBN_DEBUG -DCONF_DEBUG -m64 -DL_ENDIAN -DTERMIO -g",
+                                    "-m64 -DL_ENDIAN -DTERMIO -g",
                                     threads("-D_REENTRANT")),
         ex_libs          => add(" ","-ldl"),
         bn_ops           => "SIXTY_FOUR_BIT_LONG",
@@ -35,7 +38,7 @@ my %targets = (
     "debug-linux-pentium" => {
         inherit_from     => [ 'BASE_unix', "x86_elf_asm" ],
         cc               => "gcc",
-        cflags           => combine("-DBN_DEBUG -DREF_DEBUG -DCONF_DEBUG -DBN_CTX_DEBUG -DL_ENDIAN -g -mcpu=pentium -Wall",
+        cflags           => combine("-DL_ENDIAN -g -mcpu=pentium -Wall",
                                     threads("-D_REENTRANT")),
         ex_libs          => add(" ","-ldl"),
         bn_ops           => "BN_LLONG",
@@ -47,7 +50,7 @@ my %targets = (
     "debug-linux-ppro" => {
         inherit_from     => [ 'BASE_unix', "x86_elf_asm" ],
         cc               => "gcc",
-        cflags           => combine("-DBN_DEBUG -DREF_DEBUG -DCONF_DEBUG -DBN_CTX_DEBUG -DL_ENDIAN -g -mcpu=pentiumpro -Wall",
+        cflags           => combine("-DL_ENDIAN -g -mcpu=pentiumpro -Wall",
                                     threads("-D_REENTRANT")),
         ex_libs          => add(" ","-ldl"),
         bn_ops           => "BN_LLONG",
@@ -60,7 +63,7 @@ my %targets = (
         inherit_from     => [ 'BASE_unix', "x86_64_asm" ],
         cc               => "clang",
         cflags           => combine(join(' ', @gcc_devteam_warn),
-                                    "-Wno-error=overlength-strings -Wno-error=extended-offsetof -Wno-error=language-extension-token -Wno-error=unused-const-variable -Wstrict-overflow -Qunused-arguments -DBN_DEBUG -DCONF_DEBUG -DDEBUG_UNUSED -g3 -O3 -pipe",
+                                    "-Wno-error=overlength-strings -Wno-error=extended-offsetof -Wno-error=language-extension-token -Wno-error=unused-const-variable -Wstrict-overflow -Qunused-arguments -g3 -O3 -pipe",
                                     threads("${BSDthreads}")),
         bn_ops           => "SIXTY_FOUR_BIT_LONG",
         thread_scheme    => "pthreads",
@@ -75,7 +78,7 @@ my %targets = (
         cc               => "clang",
         cflags           => combine("-arch x86_64 -DL_ENDIAN",
                                     join(' ', @gcc_devteam_warn),
-                                    "-Wno-error=overlength-strings -Wno-error=extended-offsetof -Wno-error=language-extension-token -Wno-error=unused-const-variable -Wstrict-overflow -Qunused-arguments -DBN_DEBUG -DCONF_DEBUG -DDEBUG_UNUSED -g3 -O3 -pipe",
+                                    "-Wno-error=overlength-strings -Wno-error=extended-offsetof -Wno-error=language-extension-token -Wno-error=unused-const-variable -Wstrict-overflow -Qunused-arguments -g3 -O3 -pipe",
                                     threads("${BSDthreads}")),
         sys_id           => "MACOSX",
         bn_ops           => "SIXTY_FOUR_BIT_LONG",

Configure

@@ -136,7 +136,6 @@ EOF
 # get past these.  Note that we only use these with C compilers, not with
 # C++ compilers.
 
-# DEBUG_UNUSED enables __owur (warn unused result) checks.
 # -DPEDANTIC complements -pedantic and is meant to mask code that
 # is not strictly standard-compliant and/or implementation-specific,
 # e.g. inline assembly, disregards to alignment requirements, such
@@ -150,9 +149,9 @@ EOF
 # but 'long long' type.
 
 my @gcc_devteam_warn = qw(
-    -DDEBUG_UNUSED
-    -DPEDANTIC -pedantic -Wno-long-long
+    -DPEDANTIC -pedantic -Wno-long-long -DUNUSEDRESULT_DEBUG
     -Wall
+    -Wmissing-declarations
     -Wextra
     -Wno-unused-parameter
     -Wno-missing-field-initializers

crypto/asn1/tasn_utl.c

@@ -97,9 +97,7 @@ int ossl_asn1_do_lock(ASN1_VALUE **pval, int op, const ASN1_ITEM *it)
     case -1:
         if (!CRYPTO_DOWN_REF(lck, &ret, *lock))
             return -1;  /* failed */
-#ifdef REF_PRINT
-        fprintf(stderr, "%p:%4d:%s\n", (void*)it, ret, it->sname);
-#endif
+        REF_PRINT_EX(it->sname, ret, (void *)it);
         REF_ASSERT_ISNT(ret < 0);
         if (ret == 0) {
             CRYPTO_THREAD_lock_free(*lock);

crypto/bio/bio_local.h

@@ -9,7 +9,6 @@
 
 #include "e_os.h"
 #include "internal/sockets.h"
-#include "internal/refcount.h"
 
 /* BEGIN BIO_ADDRINFO/BIO_ADDR stuff. */
 
@@ -88,6 +87,7 @@ union bio_addr_st {
 
 #include "internal/cryptlib.h"
 #include "internal/bio.h"
+#include "internal/refcount.h"
 
 typedef struct bio_f_buffer_ctx_struct {
     /*-

crypto/bn/bn_exp.c

@@ -819,7 +819,7 @@ int BN_mod_exp_mont_consttime(BIGNUM *rr, const BIGNUM *a, const BIGNUM *p,
 
         /*
          * BN_to_montgomery can contaminate words above .top [in
-         * BN_DEBUG[_DEBUG] build]...
+         * BN_DEBUG build...
          */
         for (i = am.top; i < top; i++)
             am.d[i] = 0;
@@ -924,7 +924,7 @@ int BN_mod_exp_mont_consttime(BIGNUM *rr, const BIGNUM *a, const BIGNUM *p,
 
         /*
          * BN_to_montgomery can contaminate words above .top [in
-         * BN_DEBUG[_DEBUG] build]...
+         * BN_DEBUG build...
          */
         for (i = am.top; i < top; i++)
             am.d[i] = 0;

crypto/bn/bn_gf2m.c

@@ -715,8 +715,8 @@ static int BN_GF2m_mod_inv_vartime(BIGNUM *r, const BIGNUM *a,
     ret = 1;
 
  err:
-# ifdef BN_DEBUG                /* BN_CTX_end would complain about the
-                                 * expanded form */
+# ifdef BN_DEBUG
+    /* BN_CTX_end would complain about the expanded form */
     bn_correct_top(c);
     bn_correct_top(u);
     bn_correct_top(v);

crypto/bn/bn_local.h

@@ -28,14 +28,19 @@
 /*
  * These preprocessor symbols control various aspects of the bignum headers
  * and library code. They're not defined by any "normal" configuration, as
- * they are intended for development and testing purposes. NB: defining all
- * three can be useful for debugging application code as well as openssl
+ * they are intended for development and testing purposes. NB: defining
+ * them can be useful for debugging application code as well as openssl
  * itself. BN_DEBUG - turn on various debugging alterations to the bignum
- * code BN_DEBUG_RAND - uses random poisoning of unused words to trip up
- * mismanagement of bignum internals. You must also define BN_DEBUG.
+ * code BN_RAND_DEBUG - uses random poisoning of unused words to trip up
+ * mismanagement of bignum internals. Enable BN_RAND_DEBUG is known to
+ * break some of the OpenSSL tests.
  */
-/* #define BN_DEBUG */
-/* #define BN_DEBUG_RAND */
+# if defined(BN_RAND_DEBUG) && !defined(BN_DEBUG)
+#  define BN_DEBUG
+# endif
+# if defined(BN_RAND_DEBUG)
+#  include <openssl/rand.h>
+# endif
 
 # ifndef OPENSSL_SMALL_FOOTPRINT
 #  define BN_MUL_COMBA
@@ -127,7 +132,7 @@
  *   bn_check_top() is as before.
  * - if BN_DEBUG *is* defined;
  *   - bn_check_top() tries to pollute unused words even if the bignum 'top' is
- *     consistent. (ed: only if BN_DEBUG_RAND is defined)
+ *     consistent. (ed: only if BN_RAND_DEBUG is defined)
  *   - bn_fix_top() maps to bn_check_top() rather than "fixing" anything.
  * The idea is to have debug builds flag up inconsistent bignums when they
  * occur. If that occurs in a bn_fix_top(), we examine the code in question; if
@@ -153,7 +158,7 @@
  * all operations manipulating the bit in question in non-BN_DEBUG build.
  */
 #  define BN_FLG_FIXED_TOP 0x10000
-#  ifdef BN_DEBUG_RAND
+#  ifdef BN_RAND_DEBUG
 #   define bn_pollute(a) \
         do { \
             const BIGNUM *_bnum1 = (a); \
@@ -164,7 +169,7 @@
                  * wouldn't be constructed with top!=dmax. */ \
                 BN_ULONG *_not_const; \
                 memcpy(&_not_const, &_bnum1->d, sizeof(_not_const)); \
-                RAND_bytes(&_tmp_char, 1); /* Debug only - safe to ignore error return */\
+                (void)RAND_bytes(&_tmp_char, 1); /* Debug only - safe to ignore error return */\
                 memset(_not_const + _bnum1->top, _tmp_char, \
                        sizeof(*_not_const) * (_bnum1->dmax - _bnum1->top)); \
             } \
@@ -451,7 +456,7 @@ unsigned __int64 _umul128(unsigned __int64 a, unsigned __int64 b,
 #  endif                        /* cpu */
 # endif                         /* OPENSSL_NO_ASM */
 
-# ifdef BN_DEBUG_RAND
+# ifdef BN_RAND_DEBUG
 #  define bn_clear_top2max(a) \
         { \
         int      ind = (a)->dmax - (a)->top; \

crypto/ec/ecp_nistp224.c

@@ -1270,7 +1270,7 @@ void EC_nistp224_pre_comp_free(NISTP224_PRE_COMP *p)
         return;
 
     CRYPTO_DOWN_REF(&p->references, &i, p->lock);
-    REF_PRINT_COUNT("EC_nistp224", x);
+    REF_PRINT_COUNT("EC_nistp224", p);
     if (i > 0)
         return;
     REF_ASSERT_ISNT(i < 0);

crypto/ec/ecp_nistp256.c

@@ -1881,7 +1881,7 @@ void EC_nistp256_pre_comp_free(NISTP256_PRE_COMP *pre)
         return;
 
     CRYPTO_DOWN_REF(&pre->references, &i, pre->lock);
-    REF_PRINT_COUNT("EC_nistp256", x);
+    REF_PRINT_COUNT("EC_nistp256", pre);
     if (i > 0)
         return;
     REF_ASSERT_ISNT(i < 0);

crypto/ec/ecp_nistp521.c

@@ -1723,7 +1723,7 @@ void EC_nistp521_pre_comp_free(NISTP521_PRE_COMP *p)
         return;
 
     CRYPTO_DOWN_REF(&p->references, &i, p->lock);
-    REF_PRINT_COUNT("EC_nistp521", x);
+    REF_PRINT_COUNT("EC_nistp521", p);
     if (i > 0)
         return;
     REF_ASSERT_ISNT(i < 0);

crypto/engine/eng_init.c

@@ -34,8 +34,8 @@ int engine_unlocked_init(ENGINE *e)
          */
         e->struct_ref++;
         e->funct_ref++;
-        engine_ref_debug(e, 0, 1);
-        engine_ref_debug(e, 1, 1);
+        ENGINE_REF_PRINT(e, 0, 1);
+        ENGINE_REF_PRINT(e, 1, 1);
     }
     return to_return;
 }
@@ -57,7 +57,7 @@ int engine_unlocked_finish(ENGINE *e, int unlock_for_handlers)
      * to 0 without either calling finish().
      */
     e->funct_ref--;
-    engine_ref_debug(e, 1, -1);
+    ENGINE_REF_PRINT(e, 1, -1);
     if ((e->funct_ref == 0) && e->finish) {
         if (unlock_for_handlers)
             CRYPTO_THREAD_unlock(global_engine_lock);

crypto/engine/eng_lib.c

@@ -34,7 +34,7 @@ ENGINE *ENGINE_new(void)
         return NULL;
     }
     ret->struct_ref = 1;
-    engine_ref_debug(ret, 0, 1);
+    ENGINE_REF_PRINT(ret, 0, 1);
     if (!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_ENGINE, ret, &ret->ex_data)) {
         OPENSSL_free(ret);
         return NULL;
@@ -77,7 +77,7 @@ int engine_free_util(ENGINE *e, int not_locked)
         CRYPTO_DOWN_REF(&e->struct_ref, &i, global_engine_lock);
     else
         i = --e->struct_ref;
-    engine_ref_debug(e, 0, -1);
+    ENGINE_REF_PRINT(e, 0, -1);
     if (i > 0)
         return 1;
     REF_ASSERT_ISNT(i < 0);

crypto/engine/eng_list.c

@@ -91,7 +91,7 @@ static int engine_list_add(ENGINE *e)
      * Having the engine in the list assumes a structural reference.
      */
     e->struct_ref++;
-    engine_ref_debug(e, 0, 1);
+    ENGINE_REF_PRINT(e, 0, 1);
     /* However it came to be, e is the last item in the list. */
     engine_list_tail = e;
     e->next = NULL;
@@ -143,7 +143,7 @@ ENGINE *ENGINE_get_first(void)
     ret = engine_list_head;
     if (ret) {
         ret->struct_ref++;
-        engine_ref_debug(ret, 0, 1);
+        ENGINE_REF_PRINT(ret, 0, 1);
     }
     CRYPTO_THREAD_unlock(global_engine_lock);
     return ret;
@@ -163,7 +163,7 @@ ENGINE *ENGINE_get_last(void)
     ret = engine_list_tail;
     if (ret) {
         ret->struct_ref++;
-        engine_ref_debug(ret, 0, 1);
+        ENGINE_REF_PRINT(ret, 0, 1);
     }
     CRYPTO_THREAD_unlock(global_engine_lock);
     return ret;
@@ -183,7 +183,7 @@ ENGINE *ENGINE_get_next(ENGINE *e)
     if (ret) {
         /* Return a valid structural reference to the next ENGINE */
         ret->struct_ref++;
-        engine_ref_debug(ret, 0, 1);
+        ENGINE_REF_PRINT(ret, 0, 1);
     }
     CRYPTO_THREAD_unlock(global_engine_lock);
     /* Release the structural reference to the previous ENGINE */
@@ -204,7 +204,7 @@ ENGINE *ENGINE_get_prev(ENGINE *e)
     if (ret) {
         /* Return a valid structural reference to the next ENGINE */
         ret->struct_ref++;
-        engine_ref_debug(ret, 0, 1);
+        ENGINE_REF_PRINT(ret, 0, 1);
     }
     CRYPTO_THREAD_unlock(global_engine_lock);
     /* Release the structural reference to the previous ENGINE */
@@ -316,7 +316,7 @@ ENGINE *ENGINE_by_id(const char *id)
             }
         } else {
             iterator->struct_ref++;
-            engine_ref_debug(iterator, 0, 1);
+            ENGINE_REF_PRINT(iterator, 0, 1);
         }
     }
     CRYPTO_THREAD_unlock(global_engine_lock);

crypto/engine/eng_local.h

@@ -20,14 +20,14 @@
 extern CRYPTO_RWLOCK *global_engine_lock;
 
 /*
- * This prints the engine's pointer address (truncated to unsigned int),
- * "struct" or "funct" to indicate the reference type, the before and after
- * reference count, and the file:line-number pair. The "engine_ref_debug"
- * statements must come *after* the change.
+ * This prints the engine's pointer address, "struct" or "funct" to
+ * indicate the reference type, the before and after reference count, and
+ * the file:line-number pair. The "ENGINE_REF_PRINT" statements must come
+ * *after* the change.
  */
-# define engine_ref_debug(e, isfunct, diff)                             \
+# define ENGINE_REF_PRINT(e, isfunct, diff)                             \
     OSSL_TRACE6(ENGINE_REF_COUNT,                                       \
-               "engine: %p %s from %d to %d (%s:%d)\n",               \
+               "engine: %p %s from %d to %d (%s:%d)\n",                 \
                (void *)(e), (isfunct ? "funct" : "struct"),             \
                ((isfunct)                                               \
                 ? ((e)->funct_ref - (diff))                             \

crypto/engine/tb_asnmth.c

@@ -206,7 +206,7 @@ const EVP_PKEY_ASN1_METHOD *ENGINE_pkey_asn1_find_str(ENGINE **pe,
     /* If found obtain a structural reference to engine */
     if (fstr.e) {
         fstr.e->struct_ref++;
-        engine_ref_debug(fstr.e, 0, 1);
+        ENGINE_REF_PRINT(fstr.e, 0, 1);
     }
     *pe = fstr.e;
     CRYPTO_THREAD_unlock(global_engine_lock);

crypto/trace.c

@@ -16,6 +16,7 @@
 #include <openssl/trace.h>
 #include "internal/bio.h"
 #include "internal/nelem.h"
+#include "internal/refcount.h"
 #include "crypto/cryptlib.h"
 
 #include "e_os.h"                /* strcasecmp for Windows */
@@ -138,6 +139,7 @@ static const struct trace_category_st trace_categories[] = {
     TRACE_CATEGORY_(STORE),
     TRACE_CATEGORY_(DECODER),
     TRACE_CATEGORY_(ENCODER),
+    TRACE_CATEGORY_(REF_COUNT)
 };
 
 const char *OSSL_trace_get_category_name(int num)

include/internal/refcount.h

@@ -11,13 +11,7 @@
 # pragma once
 
 # include <openssl/e_os2.h>
-
-/* Used to checking reference counts, most while doing perl5 stuff :-) */
-# if defined(OPENSSL_NO_STDIO)
-#  if defined(REF_PRINT)
-#   error "REF_PRINT requires stdio"
-#  endif
-# endif
+# include <openssl/trace.h>
 
 # ifndef OPENSSL_DEV_NO_ATOMICS
 #  if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L \
@@ -176,11 +170,9 @@ typedef int CRYPTO_REF_COUNT;
 #  define REF_ASSERT_ISNT(i)
 # endif
 
-# ifdef REF_PRINT
-#  define REF_PRINT_COUNT(a, b) \
-        fprintf(stderr, "%p:%4d:%s\n", (void*)b, b->references, a)
-# else
-#  define REF_PRINT_COUNT(a, b)
-# endif
+# define REF_PRINT_EX(text, count, object) \
+    OSSL_TRACE3(REF_COUNT, "%p:%4d:%s\n", (object), (count), (text));
+# define REF_PRINT_COUNT(text, object) \
+    REF_PRINT_EX(text, object->references, (void *)object)
 
 #endif

include/openssl/e_os2.h

@@ -210,7 +210,7 @@ extern "C" {
 #  endif
 # endif
 
-# ifdef DEBUG_UNUSED
+# if defined(UNUSEDRESULT_DEBUG)
 #  define __owur __attribute__((__warn_unused_result__))
 # else
 #  define __owur

include/openssl/trace.h

@@ -56,7 +56,9 @@ extern "C" {
 # define OSSL_TRACE_CATEGORY_STORE              14
 # define OSSL_TRACE_CATEGORY_DECODER            15
 # define OSSL_TRACE_CATEGORY_ENCODER            16
-# define OSSL_TRACE_CATEGORY_NUM                17
+# define OSSL_TRACE_CATEGORY_REF_COUNT          17
+/* Count of available categories. */
+# define OSSL_TRACE_CATEGORY_NUM                18
 
 /* Returns the trace category number for the given |name| */
 int OSSL_trace_get_category_num(const char *name);