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); _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits