[OpenMP] Fix possible NULL dereferences

[AndreyChurbanov]

Summary

This patch checks pointer after all calls to strchr just in case format of the location string is broken.

Patch by Andrey Churbanov

[jdoerfert]

So, we had asserts, right? And this code is "copied" a few times all over. What I try to say is, I'm not sure this is a "fix" and it is sufficient.

[AndreyChurbanov]

In D90962#2379674, @jdoerfert wrote:

So, we had asserts, right? And this code is "copied" a few times all over. What I try to say is, I'm not sure this is a "fix" and it is sufficient.

KMP_DEBUG_ASSERT is no-op in release build. Thus this patch adds missed checks in order to make static code analyzers happy.
In real codes we haven't encountered broken source location string, but in theory this might happen and then NULL pointer could be dereferenced.

[jdoerfert]

In D90962#2379710, @AndreyChurbanov wrote:

In D90962#2379674, @jdoerfert wrote:

So, we had asserts, right? And this code is "copied" a few times all over. What I try to say is, I'm not sure this is a "fix" and it is sufficient.

KMP_DEBUG_ASSERT is no-op in release build. Thus this patch adds missed checks in order to make static code analyzers happy.
In real codes we haven't encountered broken source location string, but in theory this might happen and then NULL pointer could be dereferenced.

I don't think we should generally "make the static code analyzers happy" by swapping out reasonable asserts with some if-then-else logic for a case we don't expect to happen.
It also doesn't help us in any of the other copies of this code.

[AndreyChurbanov]

In D90962#2379750, @jdoerfert wrote:

In D90962#2379710, @AndreyChurbanov wrote:

In D90962#2379674, @jdoerfert wrote:

So, we had asserts, right? And this code is "copied" a few times all over. What I try to say is, I'm not sure this is a "fix" and it is sufficient.

KMP_DEBUG_ASSERT is no-op in release build. Thus this patch adds missed checks in order to make static code analyzers happy.
In real codes we haven't encountered broken source location string, but in theory this might happen and then NULL pointer could be dereferenced.

I don't think we should generally "make the static code analyzers happy" by swapping out reasonable asserts with some if-then-else logic for a case we don't expect to happen.
It also doesn't help us in any of the other copies of this code.

I am still not sure I got your comment, let me clarify.

What do you mean by "reasonable asserts"? As I pointed out the KMP_DEBUG_ASSERT is no-op in release build used by most people.
So there were no asserts in this code in the release build at all, which is potentially dangerous, as any call to strchr can return NULL.
We replaced possible NULL dereferencing crash with giving a bit lesser info to the tool, that sounds pretty reasonable to me.
What's wrong here?

Also what do you mean by "other copies of this code"?
I do see two occurrences of "if (s_line) s_line = strchr(...);" statements,
but I am not sure how to make this code better, as we need to find four semicolons
in the string to parse line number and column number.

[jdoerfert]

In D90962#2379819, @AndreyChurbanov wrote:

In D90962#2379750, @jdoerfert wrote:

In D90962#2379710, @AndreyChurbanov wrote:

In D90962#2379674, @jdoerfert wrote:

So, we had asserts, right? And this code is "copied" a few times all over. What I try to say is, I'm not sure this is a "fix" and it is sufficient.

KMP_DEBUG_ASSERT is no-op in release build. Thus this patch adds missed checks in order to make static code analyzers happy.
In real codes we haven't encountered broken source location string, but in theory this might happen and then NULL pointer could be dereferenced.

I don't think we should generally "make the static code analyzers happy" by swapping out reasonable asserts with some if-then-else logic for a case we don't expect to happen.
It also doesn't help us in any of the other copies of this code.

I am still not sure I got your comment, let me clarify.

What do you mean by "reasonable asserts"? As I pointed out the KMP_DEBUG_ASSERT is no-op in release build used by most people.
So there were no asserts in this code in the release build at all, which is potentially dangerous, as any call to strchr can return NULL.
We replaced possible NULL dereferencing crash with giving a bit lesser info to the tool, that sounds pretty reasonable to me.
What's wrong here?

With your argumentation *every* assert should be replaced by an if (!expr) abort(0) regardless of NDEBUG. Assertions, by design, provide
reasonable test coverage against broken invariants. If it is not an invariant, perform a proper check. If it is an invariant, the assertion
is not there to find all the runs that violated it but to determine during testing and as part of a CI if a violation sneaked in.

Also what do you mean by "other copies of this code"?
I do see two occurrences of "if (s_line) s_line = strchr(...);" statements,
but I am not sure how to make this code better, as we need to find four semicolons
in the string to parse line number and column number.

I mean for example __kmp_str_loc_init, which does gracefully handle ill-formed input (as far as I can tell).
If it doesn't, let's fix it as well. Either way, we should probably reuse it.

[AndreyChurbanov]

I mean for example __kmp_str_loc_init, which does gracefully handle ill-formed input (as far as I can tell).
If it doesn't, let's fix it as well. Either way, we should probably reuse it.

I'd vote for abandoning of this patch, rather than re-using __kmp_str_loc_init here.
On one hand the function indeed has all needed checks in place and all needed functionality,
on the other hand it is much heavier, makes a lot of unneeded for metadata reporting actions including expensive memory allocation calls.
So with its re-use the metadata reporting can slow the barriers, I am afraid. And people can finally see distorted execution profile in the tool.

So I'd vote for faster execution here as opposed to elegant coding.
If you are still unhappy with the change, let's abandon this patch.

[AndreyChurbanov]

Add function for reading numeric only values from location string. Use the function in metadata reporting for loop and single constructs.

Usages of __kmp_str_loc_init() adjusted to remove unneeded initialization/finalization of fname field of kmp_str_loc_t structure.

[jdoerfert]

I like the approach. Minor nits and one question though: Why are we talking about two line numbers and not line + column? This confuses me.

openmp/runtime/src/kmp_itt.inl

118

Why do these inputs change?

openmp/runtime/src/kmp_str.cpp

328

No "end" comment. Also, if we want to adapt to LLVM style we should put comments on their own line and make them full sentences with punctuation etc.

[AndreyChurbanov]

In D90962#2423099, @jdoerfert wrote:

I like the approach. Minor nits and one question though: Why are we talking about two line numbers and not line + column? This confuses me.

This is historical discrepancy. The definition of the ident_t structure in kmp.h says:

char const *psource; /**< String describing the source location.
                     The string is composed of semi-colon separated fields
                     which describe the source file, the function and a pair
                     of line numbers that delimit the construct. */

So the two numbers initially supposed to indicate start and end of OpenMP construct. For standalone constructs they are coincide.

Then in many places I see line/column naming of the two fields. I also was under the impression that these are line/column.
Now I see that Intel compiler sends us begin/end line of the construct, while clang sends us line/column of code block start.

Not sure what to do now with this discrepancy, the begin/end line of the code block looks preferable for tools those can consume this information.
On the other hand, such things can be hard to change...

openmp/runtime/src/kmp_itt.inl

118

Along with the cleanup of unchecked returns from external calls, I also tried to not introduce extra overhead. Looking at __kmp_str_loc_init in more details I found that it consists of two parts - parsing of locations string and forming additional structure for the file name. This additional structure is currently used in a single place in kmp_debugger.cpp. For all other places second parameter should be 0 in order to eliminate extra overhead including several extra malloc/free calls.

openmp/runtime/src/kmp_str.cpp

328

I'll work on the style.

[AndreyChurbanov]

Updated the style of newly added function.

[jdoerfert]

LGTM, I left a nit inlined but feel free to ignore.

In D90962#2423630, @AndreyChurbanov wrote:

In D90962#2423099, @jdoerfert wrote:

I like the approach. Minor nits and one question though: Why are we talking about two line numbers and not line + column? This confuses me.

This is historical discrepancy. The definition of the ident_t structure in kmp.h says:

char const *psource; /**< String describing the source location.
                     The string is composed of semi-colon separated fields
                     which describe the source file, the function and a pair
                     of line numbers that delimit the construct. */

So the two numbers initially supposed to indicate start and end of OpenMP construct. For standalone constructs they are coincide.

Then in many places I see line/column naming of the two fields. I also was under the impression that these are line/column.
Now I see that Intel compiler sends us begin/end line of the construct, while clang sends us line/column of code block start.

Not sure what to do now with this discrepancy, the begin/end line of the code block looks preferable for tools those can consume this information.
On the other hand, such things can be hard to change...

Argh, ... OK, so the situation we are in is that the frontends are not in agreement. One thing we should avoid is to mix the comments in the runtime as well, will just cause more confusion. I'd argue we can let the frontends do what they think is right and in the runtime just say "this is a number that is either the line end or column". We always treat it the same, given that we don't interpret it anyway. Tools need to be aware of the frontend to avoid confusion. WDYT? (This would mean an action item to make sure all comments (maybe also variable names) reflect the duality.)

openmp/runtime/src/kmp_itt.inl

118

Thanks for the explanation.

Nit: We could add a comment as we change the value:
/* init_frame */ 0 or even better (this is C++) /* init_frame */ false
init_frame might not be the best name either :(

[AndreyChurbanov]

In D90962#2426483, @jdoerfert wrote:

LGTM, I left a nit inlined but feel free to ignore.

...
Argh, ... OK, so the situation we are in is that the frontends are not in agreement. One thing we should avoid is to mix the comments in the runtime as well, will just cause more confusion. I'd argue we can let the frontends do what they think is right and in the runtime just say "this is a number that is either the line end or column". We always treat it the same, given that we don't interpret it anyway. Tools need to be aware of the frontend to avoid confusion. WDYT? (This would mean an action item to make sure all comments (maybe also variable names) reflect the duality.)

I've changed the type of init_fname parameter from int to bool; replaced names line_beg/line_end with line/col to be consistent with surrounding code; and changed parameter name LineEnd to LineEndOrCol, also adjusted comments on the definition of __kmp_str_loc_numbers to reflect the duality of the last field in location string.

Feel free to comment post-commit if needed.
Thanks.

[jdoerfert]

LG, thanks!

openmp/runtime/src/kmp_itt.inl

if (loc->reserved_2 == 0) {
         // barrier indexes to the high two bytes. It is OK because
         // KMP_MAX_FRAME_DOMAINS = 512.
         loc->reserved_2 |= (frm + 1); // save "new" value
 
         // Transform compiler-generated region location into the format
         // 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, 0);
         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);
 
         __itt_suppress_push(__itt_suppress_memory_errors);
         __kmp_itt_region_domains[frm] = __itt_domain_create(buff);
         __itt_suppress_pop();
 
if (loc->reserved_2 == 0) {
       }
     } else { // Region domain exists for this location
       // Check if team size was changed. Then create new region domain for this
       // location
       unsigned int frm = (loc->reserved_2 & 0x0000FFFF) - 1;
       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, 0);
         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);
 
         __itt_suppress_push(__itt_suppress_memory_errors);
         __kmp_itt_region_domains[frm] = __itt_domain_create(buff);
         __itt_suppress_pop();
 
if ((loc->reserved_2 & 0x0000FFFF) == 0) {
         // barrier indexes to the high two bytes. It is OK because
         // KMP_MAX_FRAME_DOMAINS = 512.
         loc->reserved_2 |= (frm + 1); // save "new" value
 
         // Transform compiler-generated region location into the format
         // 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, 0);
         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);
 
         __itt_suppress_push(__itt_suppress_memory_errors);
         __kmp_itt_region_domains[frm] = __itt_domain_create(buff);
         __itt_suppress_pop();
 
         __kmp_str_free(&buff);
         __kmp_str_loc_free(&str_loc);
         __kmp_itt_region_team_size[frm] = team_size;
         __itt_frame_submit_v3(__kmp_itt_region_domains[frm], NULL, begin, end);
       }
     } else { // Region domain exists for this location
       // Check if team size was changed. Then create new region domain for this
       // location
       unsigned int frm = (loc->reserved_2 & 0x0000FFFF) - 1;
       if (frm >= KMP_MAX_FRAME_DOMAINS)
         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, 0);
         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);
 
         __itt_suppress_push(__itt_suppress_memory_errors);
         __kmp_itt_region_domains[frm] = __itt_domain_create(buff);
         __itt_suppress_pop();
 
if (loc) {
             return; // loc->reserved_2 is still 0
           }
           // Save the barrier frame index to the high two bytes.
           loc->reserved_2 |= (frm + 1) << 16; // save "new" value
 
           // 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, 0);
           if (imbalance) {
             char *buff_imb = NULL;
             buff_imb = __kmp_str_format("%s$omp$barrier-imbalance:%d@%s:%d",
                                         str_loc.func, team_size, str_loc.file,
                                         str_loc.col);
             __itt_suppress_push(__itt_suppress_memory_errors);
             __kmp_itt_imbalance_domains[frm] = __itt_domain_create(buff_imb);
             __itt_suppress_pop();
if (metadata_domain == NULL) {
       string_handle_imbl = __itt_string_handle_create("omp_metadata_imbalance");
       string_handle_loop = __itt_string_handle_create("omp_metadata_loop");
       string_handle_sngl = __itt_string_handle_create("omp_metadata_single");
       __itt_suppress_pop();
     }
     __kmp_release_bootstrap_lock(&metadata_lock);
   }
 
-  // Parse line and column from psource string: ";file;func;line;col;;"
-  char *s_line;
-  char *s_col;
+  // Parse line and column from psource string: ";file;func;line;line;;"
   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_beg, line_end;
+  __kmp_str_loc_numbers(loc->psource, &line_beg, &line_end);
+  loop_data[0] = line_beg;
+  loop_data[1] = line_end;
   loop_data[2] = sched_type;
   loop_data[3] = iterations;
   loop_data[4] = chunk;
 
   __itt_metadata_add(metadata_domain, __itt_null, string_handle_loop,
                      __itt_metadata_u64, 5, loop_data);
 #endif
 } // __kmp_itt_metadata_loop
if (metadata_domain == NULL) {
       string_handle_imbl = __itt_string_handle_create("omp_metadata_imbalance");
       string_handle_loop = __itt_string_handle_create("omp_metadata_loop");
       string_handle_sngl = __itt_string_handle_create("omp_metadata_single");
       __itt_suppress_pop();
     }
     __kmp_release_bootstrap_lock(&metadata_lock);
   }
 
-  kmp_str_loc_t str_loc = __kmp_str_loc_init(loc->psource, 1);
+  int line_beg, line_end;
+  __kmp_str_loc_numbers(loc->psource, &line_beg, &line_end);
   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_beg;
+  single_data[1] = line_end;
 
   __itt_metadata_add(metadata_domain, __itt_null, string_handle_sngl,
                      __itt_metadata_u64, 2, single_data);
 #endif
 } // __kmp_itt_metadata_single
 
 // -----------------------------------------------------------------------------
 LINKAGE void __kmp_itt_region_starting(int gtid) {

openmp/runtime/src/kmp_str.h

struct kmp_str_loc {
   kmp_str_fname_t fname; // Will be initialized if init_fname is true.
   char *file;
   char *func;
   int line;
   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);
+void __kmp_str_loc_numbers(char const *psource, int *line_beg, int *line_end);
 void __kmp_str_loc_free(kmp_str_loc_t *loc);
 
 int __kmp_str_eqf(char const *lhs, char const *rhs);
 char *__kmp_str_format(char const *format, ...);
 void __kmp_str_free(char **str);
 int __kmp_str_match(char const *target, int len, char const *data);
 int __kmp_str_match_false(char const *data);
 int __kmp_str_match_true(char const *data);

openmp/runtime/src/kmp_str.cpp

if (pattern != NULL) {
     base_match = strcmp(ptrn.base, "*") == 0 ||
                  (fname->base != NULL && __kmp_str_eqf(fname->base, ptrn.base));
     __kmp_str_fname_free(&ptrn);
   }
 
   return dir_match && base_match;
 } // __kmp_str_fname_match
 
+// Get the begin/end construct lines from source location string.
+// Function is fast as it does not allocate memory for string duplicate,
+// and parses string in place.
+void __kmp_str_loc_numbers(char const *psource, int *line_beg, int *line_end) {
+  char *str;
+  KMP_DEBUG_ASSERT(psource);
+  KMP_DEBUG_ASSERT(line_beg);
+  KMP_DEBUG_ASSERT(line_end);
+  // Parse psource string: ";file;func;line;line;;" to get line and col only,
+  // skipping file and func.
+#ifdef __cplusplus
+  str = strchr(CCAST(char *, psource), ';'); // find 1-st semicolon
+#else
+  str = strchr(psource, ';'); // find 1-st semicolon
+#endif
+  // check returned pointer just in case the format of psource is broken
+  if (str)
+    str = strchr(str + 1, ';'); // find 2-nd semicolon
+  if (str)
+    str = strchr(str + 1, ';'); // find 3-rd semicolon
+  if (str) {
+    *line_beg = atoi(str + 1); // read begin line number
+    str = strchr(str + 1, ';'); // find 4-th semicolon
+  } else {
+    *line_beg = 0; // broken format of input string, cannot read number
+  }
+  if (str)
+    *line_end = atoi(str + 1); // read column number
+  else
+    *line_end = 0; // broken format of input string, cannot read number
+} // kmp_str_loc_numbers
+
 kmp_str_loc_t __kmp_str_loc_init(char const *psource, int init_fname) {
   kmp_str_loc_t loc;
 
   loc._bulk = NULL;
   loc.file = NULL;
   loc.func = NULL;
   loc.line = 0;
   loc.col = 0;