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

Fri Nov 6 13:02:37 PST 2020

AndreyChurbanov added a comment.

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?

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90962