Implement PCRE2_UNREACHABLE assertion for MS Visual C++ #465
Conversation
| @@ -1159,7 +1159,6 @@ for (i = 0; i < 2; i++) | |||
| } | |||
|
|
|||
| PCRE2_UNREACHABLE(); /* Control never reaches here */ | |||
| return PCRE2_ERROR_INTERNAL; | |||
There was a problem hiding this comment.
why was this return call removed?
this is a public API function and while this codepath shouldn't be normally reached, returning a random value instead of an error is likely to cause more problems; additionally, doing so mostly reverts a previously added fix without explanation.
There was a problem hiding this comment.
I think the reasoning was something like: this is an unreachable point in the code. So having the return statement there cannot help anything, since it will never be executed.
There was a problem hiding this comment.
PCRE2_UNREACHABLE() could be defined as a NOP as well (indeed it was for MSVC, which is why I added the return back in the previous "fix"), and therefore if this point is ever reached, code execution will just exit the function with a random return value.
Please see #465 for a real fix, and sorry I didn't catch this (not mentioned in the commit message or PR) change before it got merged, but next time it will be IMHO better to ask why the previous fix that you reverted was there to begin with.
There was a problem hiding this comment.
PCRE2_UNREACHABLE() could be defined as a NOP
That is true.
I don't think this point is worth debating, but before 'fixing' anything, I think a good first question to consider might be: Is this point in the code actually unreachable, or not? If it is truly unreachable, then it would be best not to merge #465. If it is reachable, then the next question would be: Why is it reachable? Is there nothing we can do to prevent that?
Just a thought.
There was a problem hiding this comment.
I don't think that returning a random value because of a bug that might make it reachable is a valid way to build an API.
Please, take your time to read that PR and the proposed changes, and if you still think that returning a random value instead of PCRE2_ERROR_INTERNAL (which is documented and used by other APIs to report those kind of errors already) lets discuss it there.
There was a problem hiding this comment.
Please, take your time to read that PR and the proposed changes
Yes, I did that before commenting.
Please, take your time to read that PR and the proposed changes, and if you still think that returning a random value instead of PCRE2_ERROR_INTERNAL...
Definitely, there is no way that I could advocate for returning a random value. What I would advocate for is understanding the code well enough, and testing it well enough, that we are certain that our assertions, invariants, etc. are true.
This isn't just about functions where the end of the body is unreachable. In PCRE2, there are probably thousands of pointer dereferences. Every one of those could conceivably become a NULL dereference if there is a bug somewhere. But we don't guard every pointer dereference with if (ptr == NULL) return PCRE2_ERROR_INTERNAL; Rather, both by design and by testing, we try to ensure that the code works as intended and we don't have surprise NULL pointers.
But as I said, the issue of whether to include return PCRE2_ERROR_INTERNAL or not is not big enough to be worth debating. If you prefer to have it there, I agree 100% 👍
What I do not personally agree with, and encourage you to reconsider, is deleting the assertion in that function. If you do so, it will be more likely that bugs will creep in and then users will actually receive PCRE2_ERROR_INTERNAL, rather than the bugs being caught and fixed before they get into a public release.
There was a problem hiding this comment.
Ah, Carlo, here's another (tangentially related) thought...
You mentioned that before I added the implementation of PCRE_UNREACHABLE which is based on __assume, it was a no-op with VC++.
How about this: If the target host doesn't have __builtin_unreachable, and it doesn't have __assume, and if PCRE2_DEBUG is defined, how about we implement PCRE2_UNREACHABLE using abort()?
That would ensure that all debug builds of PCRE2, regardless of compiler, will abort with an error if "unreachable" code is reached.
There was a problem hiding this comment.
That would ensure that all debug builds of PCRE2, regardless of compiler, will abort with an error if "unreachable" code is reached.
I am finally understanding why those asserts looked so strange to begin with; you never understood the implementation well and only meant them as a debug tool.
And now I am glad I didn't catch this and had to propose a different solution instead.
Will propose another PR (see #490) to try to address that, and hopefully this time that would be tested better before merging. After all the first implementation was broken in Windows (with a warning in our CI nonetheless), broke the build in AIX and might had not been tested much as well if the production issue it caused wasn't obvious.
Although I am starting to believe you really considered worth exposing production PCRE2 users to the additional crashes it would trigger?
There was a problem hiding this comment.
Although I am starting to believe you really considered worth exposing production PCRE2 users to the additional crashes it would trigger?
In answer to this question, of course it's not my intention to expose users of PCRE2, or any other OSS project, to crashes. Rather, the intention is that by adding debug assertions to this project, bugs can be more readily found and fixed before they go into production. To achieve that goal, the assertions also need to be combined with thorough fuzzing (or use of other powerful techniques, like concolic testing, which find code paths which would lead to an assertion failure).
Since 4f6c43d (Add assertion macros, use new PCRE2_UNREACHABLE assertion at unreachable points in code (PCRE2Project#446), 2024-08-28) and then again after 04dc664 (Implement PCRE2_UNREACHABLE assertion for MS Visual C++ (PCRE2Project#465), 2024-09-10), this API could return random values on failure. Remove assertion, and refactor the code slightly so it is more clear why the apparent dead code is needed.
Since 4f6c43d (Add assertion macros, use new PCRE2_UNREACHABLE assertion at unreachable points in code (PCRE2Project#446), 2024-08-28) and then again after 04dc664 (Implement PCRE2_UNREACHABLE assertion for MS Visual C++ (PCRE2Project#465), 2024-09-10), this API could return random values on failure. Remove assertion, until it could be added back in a way that wouldn't trigger a crash in non debug builds or result in the function returning without an API expected value.
Since 4f6c43d (Add assertion macros, use new PCRE2_UNREACHABLE assertion at unreachable points in code (#446), 2024-08-28) and then again after 04dc664 (Implement PCRE2_UNREACHABLE assertion for MS Visual C++ (#465), 2024-09-10), this API could return random values on failure. Remove assertion, until it could be added back in a way that wouldn't trigger a crash in non debug builds or result in the function returning without an API expected value.
Carlo pointed out that my code in a different PR was causing compiler warnings on Windows. The root cause is that
PCRE2_UNREACHABLE()was not implemented on Windows.Have I missed any other files which need to be updated?