Fix refleaks of timemoudle in PyInit_time()

python · vstinner · Mar 11, 2020 · Feb 12, 2020 · Feb 14, 2020 · Feb 12, 2020
commit 1704c00bb34369d311e7bcc34dcfd8e863909af1
diff --git a/Modules/timemodule.c b/Modules/timemodule.c
@@ -1759,52 +1759,78 @@ PyInit_time(void)

    /* Set, or reset, module variables like time.timezone */
    if (init_timezone(m) < 0) {
-        return NULL;
+        goto error;
    }

 #if defined(HAVE_CLOCK_GETTIME) || defined(HAVE_CLOCK_SETTIME) || defined(HAVE_CLOCK_GETRES)

 #ifdef CLOCK_REALTIME
-    PyModule_AddIntMacro(m, CLOCK_REALTIME);
+    if (PyModule_AddIntMacro(m, CLOCK_REALTIME) < 0) {
+        goto error;
+    }
 #endif
 #ifdef CLOCK_MONOTONIC
-    PyModule_AddIntMacro(m, CLOCK_MONOTONIC);
+    if (PyModule_AddIntMacro(m, CLOCK_MONOTONIC) < 0) {
+        goto error;
+    }
 #endif
 #ifdef CLOCK_MONOTONIC_RAW
-    PyModule_AddIntMacro(m, CLOCK_MONOTONIC_RAW);
+    if (PyModule_AddIntMacro(m, CLOCK_MONOTONIC_RAW) < 0) {
+        goto error;
+    }
 #endif
 #ifdef CLOCK_HIGHRES
-    PyModule_AddIntMacro(m, CLOCK_HIGHRES);
+    if (PyModule_AddIntMacro(m, CLOCK_HIGHRES) < 0) {
+        goto error;
+    }
 #endif
 #ifdef CLOCK_PROCESS_CPUTIME_ID
-    PyModule_AddIntMacro(m, CLOCK_PROCESS_CPUTIME_ID);
+    if (PyModule_AddIntMacro(m, CLOCK_PROCESS_CPUTIME_ID) < 0) {
+        goto error;
+    }
 #endif
 #ifdef CLOCK_THREAD_CPUTIME_ID
-    PyModule_AddIntMacro(m, CLOCK_THREAD_CPUTIME_ID);
+    if (PyModule_AddIntMacro(m, CLOCK_THREAD_CPUTIME_ID) < 0) {
+        goto error;
+    }
 #endif
 #ifdef CLOCK_PROF
-    PyModule_AddIntMacro(m, CLOCK_PROF);
+    if (PyModule_AddIntMacro(m, CLOCK_PROF) < 0) {
+        goto error;
+    }
 #endif
 #ifdef CLOCK_BOOTTIME
-    PyModule_AddIntMacro(m, CLOCK_BOOTTIME);
+    if (PyModule_AddIntMacro(m, CLOCK_BOOTTIME) < 0) {
+        goto error;
+    }
 #endif
 #ifdef CLOCK_UPTIME
-    PyModule_AddIntMacro(m, CLOCK_UPTIME);
+    if (PyModule_AddIntMacro(m, CLOCK_UPTIME) < 0) {
+        goto error;
+    }
 #endif
 #ifdef CLOCK_UPTIME_RAW
-    PyModule_AddIntMacro(m, CLOCK_UPTIME_RAW);
+    if (PyModule_AddIntMacro(m, CLOCK_UPTIME_RAW) < 0) {
+        goto error;
+    }
 #endif

 #endif  /* defined(HAVE_CLOCK_GETTIME) || defined(HAVE_CLOCK_SETTIME) || defined(HAVE_CLOCK_GETRES) */

    if (!initialized) {
        if (PyStructSequence_InitType2(&StructTimeType,
-                                       &struct_time_type_desc) < 0)
-            return NULL;
+                                       &struct_time_type_desc) < 0) {
+            goto error;
+        }
+    }
+    if (PyModule_AddIntConstant(m, "_STRUCT_TM_ITEMS", 11)) {
+        goto error;
    }
    Py_INCREF(&StructTimeType);
-    PyModule_AddIntConstant(m, "_STRUCT_TM_ITEMS", 11);
-    PyModule_AddObject(m, "struct_time", (PyObject*) &StructTimeType);
+    if (PyModule_AddObject(m, "struct_time", (PyObject*) &StructTimeType)) {
+        Py_DECREF(&StructTimeType);
+        goto error;
+    }
    initialized = 1;

 #if defined(__linux__) && !defined(__GLIBC__)
@@ -1815,9 +1841,13 @@ PyInit_time(void)
 #endif

    if (PyErr_Occurred()) {
-        return NULL;
+        goto error;
    }
    return m;
+
+error:
+    Py_DECREF(m);
+    return NULL;
 }

 /* Implement pysleep() for various platforms.
-Original file line number
+Diff line change
@@ -1759,52 +1759,78 @@ PyInit_time(void)
         /* Set, or reset, module variables like time.timezone */
         if (init_timezone(m) < 0) {
-            return NULL;
+            goto error;
         }
     #if defined(HAVE_CLOCK_GETTIME) || defined(HAVE_CLOCK_SETTIME) || defined(HAVE_CLOCK_GETRES)
     #ifdef CLOCK_REALTIME
-        PyModule_AddIntMacro(m, CLOCK_REALTIME);
+        if (PyModule_AddIntMacro(m, CLOCK_REALTIME) < 0) {
             Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            pganssle
  

      

      

      


        Feb 12, 2020


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      Is the reason that you need goto in every one of these blocks that PyErr_Occcurred() will get reset if you make other PyModule_AddIntMacro calls that don't fail along the way?
Either way, I'm mildly tempted to suggest adding a wrapper macro like:
#define _PyModule_AddIntMacroOrGotoError(m, macro) \
    if (PyModule_AddIntMacro(m, macro) < 0) { goto error; }
But I dunno. I'm torn between the principles of "don't repeat yourself" and "don't write a preprocessor macro with a goto in it (you monster)".
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
            shihai1991 reacted with laugh emoji
      
        
            
    All reactions
  


          
              
                😄
                  1 reaction
              
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            brandtbucher
  

      

      

      


        Feb 12, 2020


      
  •

  
    
      
        
          edited
          
        
        
      
    
    
      
  
        
      Loading


  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  
    
  


    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      Between the two options, I'm personally +0 on repetition. Incorrect error handling for the whole PyModule_Add family of functions is rampant in the stdlib (see my work in bpo-38823), and I think reinforcing the correct usage is good to have to keep people from repeating the same old patterns (especially in outside projects).
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  Author


  

  
    
      

      
            shihai1991
  

      

      

      


        Feb 13, 2020


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      
Is the reason that you need goto in every one of these blocks that PyErr_Occcurred() will get reset if you make other PyModule_AddIntMacro calls that don't fail along the way?

No, I just try to make the Py_DECREF(m);return NULL; looks like a common processing item.

I think reinforcing the correct usage is good to have to keep people from repeating the same old patterns

+1
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            vstinner
  

      

      

      


        Mar 11, 2020


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      I dislike macros hiding a goto. A goto statement is dangerous, I prefer to make it explicit. The "if (...) { goto error; }" is a very common pattern in Python, especially in initialization code.
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
+            goto error;
+        }
     #endif
     #ifdef CLOCK_MONOTONIC
-        PyModule_AddIntMacro(m, CLOCK_MONOTONIC);
+        if (PyModule_AddIntMacro(m, CLOCK_MONOTONIC) < 0) {
+            goto error;
+        }
     #endif
     #ifdef CLOCK_MONOTONIC_RAW
-        PyModule_AddIntMacro(m, CLOCK_MONOTONIC_RAW);
+        if (PyModule_AddIntMacro(m, CLOCK_MONOTONIC_RAW) < 0) {
+            goto error;
+        }
     #endif
     #ifdef CLOCK_HIGHRES
-        PyModule_AddIntMacro(m, CLOCK_HIGHRES);
+        if (PyModule_AddIntMacro(m, CLOCK_HIGHRES) < 0) {
+            goto error;
+        }
     #endif
     #ifdef CLOCK_PROCESS_CPUTIME_ID
-        PyModule_AddIntMacro(m, CLOCK_PROCESS_CPUTIME_ID);
+        if (PyModule_AddIntMacro(m, CLOCK_PROCESS_CPUTIME_ID) < 0) {
+            goto error;
+        }
     #endif
     #ifdef CLOCK_THREAD_CPUTIME_ID
-        PyModule_AddIntMacro(m, CLOCK_THREAD_CPUTIME_ID);
+        if (PyModule_AddIntMacro(m, CLOCK_THREAD_CPUTIME_ID) < 0) {
+            goto error;
+        }
     #endif
     #ifdef CLOCK_PROF
-        PyModule_AddIntMacro(m, CLOCK_PROF);
+        if (PyModule_AddIntMacro(m, CLOCK_PROF) < 0) {
+            goto error;
+        }
     #endif
     #ifdef CLOCK_BOOTTIME
-        PyModule_AddIntMacro(m, CLOCK_BOOTTIME);
+        if (PyModule_AddIntMacro(m, CLOCK_BOOTTIME) < 0) {
+            goto error;
+        }
     #endif
     #ifdef CLOCK_UPTIME
-        PyModule_AddIntMacro(m, CLOCK_UPTIME);
+        if (PyModule_AddIntMacro(m, CLOCK_UPTIME) < 0) {
+            goto error;
+        }
     #endif
     #ifdef CLOCK_UPTIME_RAW
-        PyModule_AddIntMacro(m, CLOCK_UPTIME_RAW);
+        if (PyModule_AddIntMacro(m, CLOCK_UPTIME_RAW) < 0) {
+            goto error;
+        }
     #endif
     #endif  /* defined(HAVE_CLOCK_GETTIME) || defined(HAVE_CLOCK_SETTIME) || defined(HAVE_CLOCK_GETRES) */
         if (!initialized) {
             if (PyStructSequence_InitType2(&StructTimeType,
-                                           &struct_time_type_desc) < 0)
-                return NULL;
+                                           &struct_time_type_desc) < 0) {
+                goto error;
+            }
+        }
+        if (PyModule_AddIntConstant(m, "_STRUCT_TM_ITEMS", 11)) {
+            goto error;
         }
         Py_INCREF(&StructTimeType);
-        PyModule_AddIntConstant(m, "_STRUCT_TM_ITEMS", 11);
-        PyModule_AddObject(m, "struct_time", (PyObject*) &StructTimeType);
+        if (PyModule_AddObject(m, "struct_time", (PyObject*) &StructTimeType)) {
+            Py_DECREF(&StructTimeType);
+            goto error;
+        }
         initialized = 1;
     #if defined(__linux__) && !defined(__GLIBC__)
@@ -1815,9 +1841,13 @@ PyInit_time(void)
     #endif
         if (PyErr_Occurred()) {
                   shihai1991 marked this conversation as resolved.
              

          
            Show resolved
            Hide resolved
        

                
  
            
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.
-            return NULL;
+            goto error;
         }
         return m;
+    error:
+        Py_DECREF(m);
+        return NULL;
     }
     /* Implement pysleep() for various platforms.
-          Expand Down