[Openmp-commits] [PATCH] D90962: [OpenMP] Fix possible NULL dereferences

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added a comment.

In D90962#2379819 <https://reviews.llvm.org/D90962#2379819>, @AndreyChurbanov wrote:

> In D90962#2379750 <https://reviews.llvm.org/D90962#2379750>, @jdoerfert wrote:
>
>> In D90962#2379710 <https://reviews.llvm.org/D90962#2379710>, @AndreyChurbanov wrote:
>>
>>> In D90962#2379674 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90962/new/

https://reviews.llvm.org/D90962