[llvm-branch-commits] [openmp] 22558c8 - [OpenMP] libomp: Fix possible NULL dereferences

via llvm-branch-commits llvm-branch-commits at lists.llvm.org
Mon Dec 7 08:09:29 PST 2020


Author: AndreyChurbanov
Date: 2020-12-07T19:09:07+03:00
New Revision: 22558c8501eaf5e7547ee13fa5a009efdec6dc90

URL: https://github.com/llvm/llvm-project/commit/22558c8501eaf5e7547ee13fa5a009efdec6dc90
DIFF: https://github.com/llvm/llvm-project/commit/22558c8501eaf5e7547ee13fa5a009efdec6dc90.diff

LOG: [OpenMP] libomp: Fix possible NULL dereferences

Check pointer returned by strchr, as it can be NULL in case of broken
format of input string. Introduced new function __kmp_str_loc_numbers
for fast parsing of numbers only in the location string.
Also made some cleanup of __kmp_str_loc_init declaration and usage:
- changed type of init_fname parameter to bool;
- changed input from true to false in places where fname is not used.

Differential Revision: https://reviews.llvm.org/D90962

Added: 
    

Modified: 
    openmp/runtime/src/kmp_debugger.cpp
    openmp/runtime/src/kmp_itt.inl
    openmp/runtime/src/kmp_lock.cpp
    openmp/runtime/src/kmp_str.cpp
    openmp/runtime/src/kmp_str.h

Removed: 
    


################################################################################
diff  --git a/openmp/runtime/src/kmp_debugger.cpp b/openmp/runtime/src/kmp_debugger.cpp
index 490300f9b207..2a1f633c49c1 100644
--- a/openmp/runtime/src/kmp_debugger.cpp
+++ b/openmp/runtime/src/kmp_debugger.cpp
@@ -269,7 +269,7 @@ int __kmp_omp_num_threads(ident_t const *ident) {
   if (info->num > 0 && info->array != 0) {
     kmp_omp_nthr_item_t *items =
         (kmp_omp_nthr_item_t *)__kmp_convert_to_ptr(info->array);
-    kmp_str_loc_t loc = __kmp_str_loc_init(ident->psource, 1);
+    kmp_str_loc_t loc = __kmp_str_loc_init(ident->psource, true);
     int i;
     for (i = 0; i < info->num; ++i) {
       if (kmp_location_match(&loc, &items[i])) {

diff  --git a/openmp/runtime/src/kmp_itt.inl b/openmp/runtime/src/kmp_itt.inl
index 6257cea4faf3..09d5480284e0 100644
--- a/openmp/runtime/src/kmp_itt.inl
+++ b/openmp/runtime/src/kmp_itt.inl
@@ -115,7 +115,8 @@ LINKAGE void __kmp_itt_region_forking(int gtid, int team_size, int barriers) {
         // that the tools more or less standardized on:
         //   "<func>$omp$parallel@[file:]<line>[:<col>]"
         char *buff = NULL;
-        kmp_str_loc_t str_loc = __kmp_str_loc_init(loc->psource, 1);
+        kmp_str_loc_t str_loc =
+            __kmp_str_loc_init(loc->psource, /* init_fname */ false);
         buff = __kmp_str_format("%s$omp$parallel:%d@%s:%d:%d", str_loc.func,
                                 team_size, str_loc.file, str_loc.line,
                                 str_loc.col);
@@ -155,7 +156,8 @@ LINKAGE void __kmp_itt_region_forking(int gtid, int team_size, int barriers) {
       if ((frm < KMP_MAX_FRAME_DOMAINS) &&
           (__kmp_itt_region_team_size[frm] != team_size)) {
         char *buff = NULL;
-        kmp_str_loc_t str_loc = __kmp_str_loc_init(loc->psource, 1);
+        kmp_str_loc_t str_loc = 
+            __kmp_str_loc_init(loc->psource, /* init_fname */ false);
         buff = __kmp_str_format("%s$omp$parallel:%d@%s:%d:%d", str_loc.func,
                                 team_size, str_loc.file, str_loc.line,
                                 str_loc.col);
@@ -212,7 +214,8 @@ LINKAGE void __kmp_itt_frame_submit(int gtid, __itt_timestamp begin,
         // that the tools more or less standardized on:
         //   "<func>$omp$parallel:team_size@[file:]<line>[:<col>]"
         char *buff = NULL;
-        kmp_str_loc_t str_loc = __kmp_str_loc_init(loc->psource, 1);
+        kmp_str_loc_t str_loc = 
+            __kmp_str_loc_init(loc->psource, /* init_fname */ false);
         buff = __kmp_str_format("%s$omp$parallel:%d@%s:%d:%d", str_loc.func,
                                 team_size, str_loc.file, str_loc.line,
                                 str_loc.col);
@@ -234,7 +237,8 @@ LINKAGE void __kmp_itt_frame_submit(int gtid, __itt_timestamp begin,
         return; // something's gone wrong, returning
       if (__kmp_itt_region_team_size[frm] != team_size) {
         char *buff = NULL;
-        kmp_str_loc_t str_loc = __kmp_str_loc_init(loc->psource, 1);
+        kmp_str_loc_t str_loc = 
+            __kmp_str_loc_init(loc->psource, /* init_fname */ false);
         buff = __kmp_str_format("%s$omp$parallel:%d@%s:%d:%d", str_loc.func,
                                 team_size, str_loc.file, str_loc.line,
                                 str_loc.col);
@@ -273,7 +277,8 @@ LINKAGE void __kmp_itt_frame_submit(int gtid, __itt_timestamp begin,
           // Transform compiler-generated region location into the format
           // that the tools more or less standardized on:
           //   "<func>$omp$frame@[file:]<line>[:<col>]"
-          kmp_str_loc_t str_loc = __kmp_str_loc_init(loc->psource, 1);
+          kmp_str_loc_t str_loc = 
+              __kmp_str_loc_init(loc->psource, /* init_fname */ false);
           if (imbalance) {
             char *buff_imb = NULL;
             buff_imb = __kmp_str_format("%s$omp$barrier-imbalance:%d@%s:%d",
@@ -365,25 +370,12 @@ LINKAGE void __kmp_itt_metadata_loop(ident_t *loc, kmp_uint64 sched_type,
   }
 
   // Parse line and column from psource string: ";file;func;line;col;;"
-  char *s_line;
-  char *s_col;
   KMP_DEBUG_ASSERT(loc->psource);
-#ifdef __cplusplus
-  s_line = strchr(CCAST(char *, loc->psource), ';');
-#else
-  s_line = strchr(loc->psource, ';');
-#endif
-  KMP_DEBUG_ASSERT(s_line);
-  s_line = strchr(s_line + 1, ';'); // 2-nd semicolon
-  KMP_DEBUG_ASSERT(s_line);
-  s_line = strchr(s_line + 1, ';'); // 3-rd semicolon
-  KMP_DEBUG_ASSERT(s_line);
-  s_col = strchr(s_line + 1, ';'); // 4-th semicolon
-  KMP_DEBUG_ASSERT(s_col);
-
   kmp_uint64 loop_data[5];
-  loop_data[0] = atoi(s_line + 1); // read line
-  loop_data[1] = atoi(s_col + 1); // read column
+  int line, col;
+  __kmp_str_loc_numbers(loc->psource, &line, &col);
+  loop_data[0] = line;
+  loop_data[1] = col;
   loop_data[2] = sched_type;
   loop_data[3] = iterations;
   loop_data[4] = chunk;
@@ -409,12 +401,11 @@ LINKAGE void __kmp_itt_metadata_single(ident_t *loc) {
     __kmp_release_bootstrap_lock(&metadata_lock);
   }
 
-  kmp_str_loc_t str_loc = __kmp_str_loc_init(loc->psource, 1);
+  int line, col;
+  __kmp_str_loc_numbers(loc->psource, &line, &col);
   kmp_uint64 single_data[2];
-  single_data[0] = str_loc.line;
-  single_data[1] = str_loc.col;
-
-  __kmp_str_loc_free(&str_loc);
+  single_data[0] = line;
+  single_data[1] = col;
 
   __itt_metadata_add(metadata_domain, __itt_null, string_handle_sngl,
                      __itt_metadata_u64, 2, single_data);

diff  --git a/openmp/runtime/src/kmp_lock.cpp b/openmp/runtime/src/kmp_lock.cpp
index ee72a8610359..6fa6bf060673 100644
--- a/openmp/runtime/src/kmp_lock.cpp
+++ b/openmp/runtime/src/kmp_lock.cpp
@@ -3889,7 +3889,7 @@ void __kmp_cleanup_user_locks(void) {
       if (__kmp_env_consistency_check && (!IS_CRITICAL(lck)) &&
           ((loc = __kmp_get_user_lock_location(lck)) != NULL) &&
           (loc->psource != NULL)) {
-        kmp_str_loc_t str_loc = __kmp_str_loc_init(loc->psource, 0);
+        kmp_str_loc_t str_loc = __kmp_str_loc_init(loc->psource, false);
         KMP_WARNING(CnsLockNotDestroyed, str_loc.file, str_loc.line);
         __kmp_str_loc_free(&str_loc);
       }

diff  --git a/openmp/runtime/src/kmp_str.cpp b/openmp/runtime/src/kmp_str.cpp
index 75fd1e25f347..24526e587412 100644
--- a/openmp/runtime/src/kmp_str.cpp
+++ b/openmp/runtime/src/kmp_str.cpp
@@ -295,7 +295,54 @@ int __kmp_str_fname_match(kmp_str_fname_t const *fname, char const *pattern) {
   return dir_match && base_match;
 } // __kmp_str_fname_match
 
-kmp_str_loc_t __kmp_str_loc_init(char const *psource, int init_fname) {
+// Get the numeric fields from source location string.
+// For clang these fields are Line/Col of the start of the construct.
+// For icc these are LineBegin/LineEnd of the construct.
+// Function is fast as it does not duplicate string (which involves memory
+// allocation), and parses the string in place.
+void __kmp_str_loc_numbers(char const *Psource, int *LineBeg,
+                           int *LineEndOrCol) {
+  char *Str;
+  KMP_DEBUG_ASSERT(LineBeg);
+  KMP_DEBUG_ASSERT(LineEndOrCol);
+  // Parse Psource string ";file;func;line;line_end_or_column;;" to get
+  // numbers only, skipping string fields "file" and "func".
+
+  // Find 1-st semicolon.
+  KMP_DEBUG_ASSERT(Psource);
+#ifdef __cplusplus
+  Str = strchr(CCAST(char *, Psource), ';');
+#else
+  Str = strchr(Psource, ';');
+#endif
+  // Check returned pointer to see if the format of Psource is broken.
+  if (Str) {
+    // Find 2-nd semicolon.
+    Str = strchr(Str + 1, ';');
+  }
+  if (Str) {
+    // Find 3-rd semicolon.
+    Str = strchr(Str + 1, ';');
+  }
+  if (Str) {
+    // Read begin line number.
+    *LineBeg = atoi(Str + 1);
+    // Find 4-th semicolon.
+    Str = strchr(Str + 1, ';');
+  } else {
+    // Broken format of input string, cannot read the number.
+    *LineBeg = 0;
+  }
+  if (Str) {
+    // Read end line number.
+    *LineEndOrCol = atoi(Str + 1);
+  } else {
+    // Broken format of input string, cannot read the number.
+    *LineEndOrCol = 0;
+  }
+}
+
+kmp_str_loc_t __kmp_str_loc_init(char const *psource, bool init_fname) {
   kmp_str_loc_t loc;
 
   loc._bulk = NULL;

diff  --git a/openmp/runtime/src/kmp_str.h b/openmp/runtime/src/kmp_str.h
index 9e669bbe4742..98e2f4a457fe 100644
--- a/openmp/runtime/src/kmp_str.h
+++ b/openmp/runtime/src/kmp_str.h
@@ -81,7 +81,7 @@ int __kmp_str_fname_match(kmp_str_fname_t const *fname, char const *pattern);
    structure keeps source location in more convenient form.
    Usage:
 
-   kmp_str_loc_t loc = __kmp_str_loc_init( ident->psource, 0 );
+   kmp_str_loc_t loc = __kmp_str_loc_init(ident->psource, false);
    // use loc.file, loc.func, loc.line, loc.col.
    // loc.fname is available if second argument of __kmp_str_loc_init is true.
    __kmp_str_loc_free( & loc );
@@ -98,7 +98,8 @@ struct kmp_str_loc {
   int col;
 }; // struct kmp_str_loc
 typedef struct kmp_str_loc kmp_str_loc_t;
-kmp_str_loc_t __kmp_str_loc_init(char const *psource, int init_fname);
+kmp_str_loc_t __kmp_str_loc_init(char const *psource, bool init_fname);
+void __kmp_str_loc_numbers(char const *Psource, int *Line, int *Col);
 void __kmp_str_loc_free(kmp_str_loc_t *loc);
 
 int __kmp_str_eqf(char const *lhs, char const *rhs);


        


More information about the llvm-branch-commits mailing list