Project

General

Profile

Actions

Bug #19028

closed

GCC12 Introduces new warn flags `-Wuse-after-free`

Added by eightbitraptor (Matthew Valentine-House) over 1 year ago. Updated over 1 year ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:110133]

Description

GCC 12 introduced a new warning flag -Wuse-after-free which attempts to warn about uses of pointers to dynamically allocated objects that have been rendered indeterminate by a call to a deallocation function

Details of the levels are in the C++ Dialect Options section of the GCC documentation.

Compiling with -Wall uses the default setting of 2 for this flag. Which warns on the TRY_WITH_GC macro defined in gc.c

gc.c: In function ‘objspace_xrealloc’:                                                                             
gc.c:12213:33: warning: pointer ‘ptr’ may be used after ‘realloc’ [-Wuse-after-free]                               
12213 |     TRY_WITH_GC(new_size, mem = realloc(ptr, new_size));                                                   
      |                                 ^~~~~~~~~~~~~~~~~~~~~~                                                     
gc.c:12123:19: note: in definition of macro ‘TRY_WITH_GC’                                                          
12123 |         else if ((expr)) {                                   \                                             
      |                   ^~~~                                                                                     
In file included from ./include/ruby/defines.h:72,                                                                 
                 from ./include/ruby/ruby.h:25,                                                                    
                 from constant.h:13,                                                                               
                 from gc.c:97:                                                                                     
gc.c:12213:33: note: call to ‘realloc’ here                                                                        
12213 |     TRY_WITH_GC(new_size, mem = realloc(ptr, new_size));                                                   
      |                                 ^~~~~~~~~~~~~~~~~~~~~~                                                                                                                                                                        
./include/ruby/backward/2/assume.h:43:46: note: in definition of macro ‘RB_LIKELY’                                 
   43 | # define RB_LIKELY(x)   (__builtin_expect(!!(x), 1))                                                       
      |                                              ^                                                             
gc.c:12116:13: note: in expansion of macro ‘LIKELY’ 
12116 |         if (LIKELY((expr))) {                                \                                             
      |             ^~~~~~                       
gc.c:12213:5: note: in expansion of macro ‘TRY_WITH_GC’
12213 |     TRY_WITH_GC(new_size, mem = realloc(ptr, new_size));                                                   
      |     ^~~~~~~~~~~            

My understanding is that if realloc returns a null pointer then the memory requested for reallocation is guaranteed to not be touched (according to the Open Group - thank you @nobu (Nobuyoshi Nakada) for bringing this to my attention).

Given that this is a new warning, my proposed solution is to lower the level down to the base level 1. This will only warn on unconditional calls to deallocation functions or successful calls to realloc.

I've opened a PR that sets -Wuse-after-free=1 only for GCC versions > 11. An alternative approach might be to use #pragma GCC diagnostic to suppress this just for GCC but I opted for what I thought was the easiest fix to start with.

Github PR #6465

Actions #1

Updated by eightbitraptor (Matthew Valentine-House) over 1 year ago

  • Description updated (diff)
Actions #2

Updated by eightbitraptor (Matthew Valentine-House) over 1 year ago

  • Description updated (diff)

Updated by mame (Yusuke Endoh) over 1 year ago

This seems to be this bug in gcc. I don't see it getting fixed soon. +1 for -Wuse-after-free=1.

Updated by nobu (Nobuyoshi Nakada) over 1 year ago

GCC 12 on GitHub Actions doesn’t show that warning.
https://github.com/ruby/ruby/actions/runs/3146751201/jobs/5115549022#step:13:130

I’m not sure what differs.

Updated by mame (Yusuke Endoh) over 1 year ago

I am not able to repro with gcc-12 (Ubuntu 12.1.0-2ubuntu1~22.04) 12.1.0 and x86_64-linux.

Updated by eightbitraptor (Matthew Valentine-House) over 1 year ago

nobu (Nobuyoshi Nakada) wrote in #note-4:

GCC 12 on GitHub Actions doesn’t show that warning.
https://github.com/ruby/ruby/actions/runs/3146751201/jobs/5115549022#step:13:130

I’m not sure what differs.

mame (Yusuke Endoh) wrote in #note-5:

I am not able to repro with gcc-12 (Ubuntu 12.1.0-2ubuntu1~22.04) 12.1.0 and x86_64-linux.

Apologies. I had a closer look into this and realised that this warning only occurs when optflags=-O0.

With O0 I can successfully repro this on:

  • gcc version 12.2.1 20220819 (Red Hat 12.2.1-2) (GCC) on Fedora 36 - where I originally saw this warning.
  • gcc version 12.1.0 (Ubuntu 12.1.0-2ubuntu1~22.04)
  • gcc version 12.2.0 (Ubuntu 12.2.0-3ubuntu1) on Ubuntu Kinetic Kudu (development branch) 22.10

I don't know if we should justify lowering a warning level globally for a warning that only appears in unoptimized code - what do you think?

Updated by nobu (Nobuyoshi Nakada) over 1 year ago

I see, what about this?

diff --git a/gc.c b/gc.c
index ac5ed1e7cba..a22da5482be 100644
--- a/gc.c
+++ b/gc.c
@@ -12102,10 +12102,18 @@ objspace_malloc_fixup(rb_objspace_t *objspace, void *mem, size_t size)
 # define RB_BUG_INSTEAD_OF_RB_MEMERROR 0
 #endif
 
-#define GC_MEMERROR(...) \
+#define GC_MEMERROR(...) /* NORETURN */ \
     ((RB_BUG_INSTEAD_OF_RB_MEMERROR+0) ? rb_bug("" __VA_ARGS__) : rb_memerror())
 
-#define TRY_WITH_GC(siz, expr) do {                          \
+#if RBIMPL_COMPILER_SINCE(GCC, 12, 0, 0) && !defined(__OPTIMIZE__)
+# define DISABLE_GNUC_USE_AFTRE_FREE_WARNING() \
+    RBIMPL_WARNING_IGNORED(-Wuse-after-free)
+#else
+# define DISABLE_GNUC_USE_AFTRE_FREE_WARNING()
+#endif
+
+#define TRY_WITH_GC(siz, expr) (siz, expr, (void)0)
+#define TRY_WITH_GC_WITHOUT_WARNING(siz, expr, warn2nd) do { \
         const gc_profile_record_flag gpr =                   \
             GPR_FLAG_FULL_MARK           |                   \
             GPR_FLAG_IMMEDIATE_MARK      |                   \
@@ -12114,16 +12122,19 @@ objspace_malloc_fixup(rb_objspace_t *objspace, void *mem, size_t size)
         objspace_malloc_gc_stress(objspace);                 \
                                                              \
         if (LIKELY((expr))) {                                \
-            /* Success on 1st try */                         \
+            break; /* Success on 1st try */                  \
         }                                                    \
-        else if (!garbage_collect_with_gvl(objspace, gpr)) { \
+        if (!garbage_collect_with_gvl(objspace, gpr)) {      \
             /* @shyouhei thinks this doesn't happen */       \
             GC_MEMERROR("TRY_WITH_GC: could not GC");        \
         }                                                    \
-        else if ((expr)) {                                   \
-            /* Success on 2nd try */                         \
+        RBIMPL_WARNING_PUSH();                               \
+        warn2nd;                                             \
+        if ((expr)) {                                        \
+            break; /* Success on 2nd try */                  \
         }                                                    \
-        else {                                               \
+        RBIMPL_WARNING_POP();                                \
+        {                                                    \
             GC_MEMERROR("TRY_WITH_GC: could not allocate:"   \
                         "%"PRIdSIZE" bytes for %s",          \
                         siz, # expr);                        \
@@ -12210,7 +12221,8 @@ objspace_xrealloc(rb_objspace_t *objspace, void *ptr, size_t new_size, size_t ol
 #endif
 
     old_size = objspace_malloc_size(objspace, ptr, old_size);
-    TRY_WITH_GC(new_size, mem = realloc(ptr, new_size));
+    TRY_WITH_GC_WITHOUT_WARNING(new_size, mem = realloc(ptr, new_size),
+                                DISABLE_GNUC_USE_AFTRE_FREE_WARNING());
     new_size = objspace_malloc_size(objspace, mem, new_size);
 
 #if CALC_EXACT_MALLOC_SIZE

Updated by eightbitraptor (Matthew Valentine-House) over 1 year ago

nobu (Nobuyoshi Nakada) wrote in #note-7:

I see, what about this?

I like this approach. It makes sense to just disable the warning explicitly for code that we know is correct. Thanks

Updated by nobu (Nobuyoshi Nakada) over 1 year ago

I found the warning is suppressed by a statement-expression.
Don't ask why 🤪
https://github.com/ruby/ruby/compare/master...nobu:ruby:bug/19028-realloc-warning

Actions #10

Updated by nobu (Nobuyoshi Nakada) over 1 year ago

  • Status changed from Open to Closed

Applied in changeset git|40ceceb1a5b63029a4d1434d2d20dfa09cdb295f.


[Bug #19028] Suppress GCC 12 -Wuse-after-free false warning

GCC 12 introduced a new warning flag -Wuse-after-free, however it
has a false positive at realloc when optimization is disabled, since
the memory requested for reallocation is guaranteed to not be touched.
This workaround is very unclear why the false warning is suppressed by
a statement-expression GCC extension.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0