[Openmp-commits] [PATCH] D26786: Fix memory leaks of buffer allocated in __kmp_str_format()

[Andrey Churbanov via Openmp-commits]

AndreyChurbanov requested changes to this revision.
AndreyChurbanov added a comment.
This revision now requires changes to proceed.

1. __kmp_msg(kmp_ms_fatal,...) never returns, and it cleans up its arguments inside.  Thus trying to clean its argument once more time is an error.
2. __kmp_msg(kmp_ms_warning,...) cleans its arguments in case it tries to print anything, and doesn't clean in case it returns immediately. Thus the added cleanup action should be conditional, e.g.

  if(__kmp_generate_warnings == kmp_warnings_off)
      __kmp_str_free(&err_code.str);

I tried to mark all changes those need to be under condition, and those are OK.  All the changes for fatal messages should be reverted.



================
Comment at: runtime/src/kmp_affinity.h:61
+                __kmp_msg(kmp_ms_fatal, KMP_MSG( FatalSysError ), err_code, __kmp_msg_null);
+                __kmp_str_free(&err_code.str);
             }
----------------
This change is bad, as well as all other changes for __kmp_msg(kmp_ms_fatal,...).


================
Comment at: runtime/src/kmp_environment.c:583
     KMP_INTERNAL_FREE( (void *) block->vars );
-    KMP_INTERNAL_FREE( (void *) block->bulk );
+    __kmp_str_free(&(block->bulk));
 
----------------
This change is OK, as the __kmp_str_free frees and nullifies parameter.


================
Comment at: runtime/src/kmp_i18n.c:160
 	);
+  __kmp_str_free(&err_code.str);
 	KMP_INFORM( WillUseDefaultMessages );
----------------
Needs condition.


================
Comment at: runtime/src/kmp_i18n.c:424
+      );
+  __kmp_str_free(&err_msg.str);
+  KMP_INFORM( WillUseDefaultMessages );
----------------
Needs condition.


================
Comment at: runtime/src/kmp_i18n.c:827
             int    size   = 2048;
-            // TODO: Add checking result of malloc().
             char * buffer = (char *) KMP_INTERNAL_MALLOC( size );
----------------
OK to remove the comment.


================
Comment at: runtime/src/kmp_i18n.c:835
             }
+            int    rc;
             rc = strerror_r( err, buffer, size );
----------------
I think keeping all declarations together looks better that moving one here. Why this change?


================
Comment at: runtime/src/kmp_i18n.c:938
     fmsg = __kmp_msg_format( format, message.num, message.str );
-    KMP_INTERNAL_FREE( (void *) message.str );
+    __kmp_str_free(&message.str);
     __kmp_str_buf_cat( & buffer, fmsg.str, fmsg.len );
----------------
These two changes are OK.


================
Comment at: runtime/src/kmp_i18n.c:964
         fmsg = __kmp_msg_format( format, message.num, message.str );
-        KMP_INTERNAL_FREE( (void *) message.str );
+        __kmp_str_free(&message.str);
         __kmp_str_buf_cat( & buffer, fmsg.str, fmsg.len );
----------------
These two are OK as well.


================
Comment at: runtime/src/kmp_itt.c:110
+                __kmp_msg( kmp_ms_warning, KMP_MSG( IttLoadLibFailed, library ), err_code, __kmp_msg_null );
+                __kmp_str_free(&err_code.str);
             #else
----------------
Needs condition.


================
Comment at: runtime/src/kmp_itt.c:115
+                __kmp_msg( kmp_ms_warning, KMP_MSG( IttLoadLibFailed, library ), err_code, __kmp_msg_null );
+                __kmp_str_free(&err_code.str);
             #endif
----------------
Needs condition.


================
Comment at: runtime/src/kmp_itt.c:139
+            __kmp_msg( kmp_ms_warning, KMP_MSG( CantGetEnvVar, var ), err_code, __kmp_msg_null );
+            __kmp_str_free(&err_code.str);
         } break;
----------------
Needs condition.


================
Comment at: runtime/src/kmp_itt.c:146
+            __kmp_msg( kmp_ms_warning, KMP_MSG( IttFunctionError, func ), err_code, __kmp_msg_null );
+            __kmp_str_free(&err_code.str);
         } break;
----------------
Needs condition.


================
Comment at: runtime/src/kmp_settings.c:363
 ) {
-    KMP_INTERNAL_FREE( (void *) * out );
+    __kmp_str_free(out);
     * out = __kmp_str_format( "%s", value );
----------------
This is OK.


================
Comment at: runtime/src/kmp_settings.c:421
     int hasSuffix;
-    KMP_INTERNAL_FREE( (void *) * out );
+    __kmp_str_free(out);
     t = (char *) strrchr(value, '.');
----------------
These two are OK.


================
Comment at: runtime/src/kmp_settings.c:2238
 
-    KMP_INTERNAL_FREE( buffer );
+    __kmp_str_free((const char **) &buffer);
 
----------------
This is OK.


================
Comment at: runtime/src/kmp_str.c:423
     __kmp_str_fname_free( & loc->fname );
-    KMP_INTERNAL_FREE( loc->_bulk );
-    loc->_bulk = NULL;
+    __kmp_str_free((const char **) &(loc->_bulk));
     loc->file  = NULL;
----------------
This is OK.


================
Comment at: runtime/src/kmp_str.c:481
 
-// TODO: Find and replace all regular free() with __kmp_str_free().
-
----------------
OK


================
Comment at: runtime/src/z_Linux_util.c:175
             );
+            __kmp_str_free(&err_code.str);
         }
----------------
Needs condition.


================
Comment at: runtime/src/z_Linux_util.c:205
+                            );
+                    __kmp_str_free(&err_code.str);
                 }
----------------
Needs condition.


================
Comment at: runtime/src/z_Linux_util.c:255
+                            );
+                    __kmp_str_free(&err_code.str);
                 }
----------------
Needs condition.


================
Comment at: runtime/src/z_Linux_util.c:288
+                            );
+                    __kmp_str_free(&err_code.str);
                 }
----------------
Needs condition.


================
Comment at: runtime/src/z_Linux_util.c:732
+                            );
+                    __kmp_str_free(&err_code.str);
                 }; // if
----------------
Needs condition.


================
Comment at: runtime/src/z_Linux_util.c:989
+        __kmp_msg(kmp_ms_warning, KMP_MSG( CantDestroyThreadAttrs ), err_code, __kmp_msg_null);
+        __kmp_str_free(&err_code.str);
     }; // if
----------------
Needs condition.


================
Comment at: runtime/src/z_Linux_util.c:1097
+                    );
+            __kmp_str_free(&err_code.str);
         }; // if
----------------
Needs condition.


================
Comment at: runtime/src/z_Linux_util.c:1166
             );
+            __kmp_str_free(&err_code.str);
         }; // if
----------------
Needs condition.


================
Comment at: runtime/src/z_Windows_NT_util.c:570
                 );
+                __kmp_str_free(&err_code.str);
             }
----------------
Needs condition.


https://reviews.llvm.org/D26786