Skip to content

Prevent null pointer dereference in bwriteprotect macro usage#20

Merged
msteinert merged 2 commits into
mainfrom
null-pointer-abort
Aug 19, 2025
Merged

Prevent null pointer dereference in bwriteprotect macro usage#20
msteinert merged 2 commits into
mainfrom
null-pointer-abort

Conversation

@rdmark

@rdmark rdmark commented Aug 12, 2025

Copy link
Copy Markdown
Collaborator

Aborts instead of asserts before using the bwriteprotect macro with a null parameter, to avoid null pointer dereference

Aborts instead of asserts before using the bwriteprotect macro
with a null parameter, to avoid null pointer dereference
@rdmark

rdmark commented Aug 12, 2025

Copy link
Copy Markdown
Collaborator Author

@msteinert Sonar reports now one outstanding bug, in the bwriteprotect macro, which seems legit to my eyes. This PR should protect against this scenario, but Sonar is now reporting 10 new bugs that are clearly spurious, unless I'm overlooking something obvious. Would you mind taking a look at the latest report and tell me what you think?

@msteinert

Copy link
Copy Markdown
Owner

It seems like the analyzer forgot how return works in this case?

image

I'm usually reluctant to say the analyzer is wrong, but it does seem that way in this case.

@msteinert

Copy link
Copy Markdown
Owner

I wonder if the static analyzer would be less confused if the conditions were combined?

if (b0 == NULL || res == NULL) {
        ck_abort();
        return; /* Just a safeguard */
}

...

Granted, this is less useful for debugging a failure.

@rdmark
rdmark force-pushed the null-pointer-abort branch 3 times, most recently from ed89a0e to d759042 Compare August 13, 2025 04:56
@rdmark
rdmark force-pushed the null-pointer-abort branch from d759042 to bda23fc Compare August 13, 2025 04:58
@sonarqubecloud

Copy link
Copy Markdown

@rdmark
rdmark requested a review from msteinert August 13, 2025 05:15
@rdmark

rdmark commented Aug 13, 2025

Copy link
Copy Markdown
Collaborator Author

This approach actually appeased the static analyzer for three of the cases. I've pushed a commit now that slightly refactors the rest with else statements that cleanly splits out the branches.

Still not sure why Sonar picks up on these particular branches.

@msteinert
msteinert merged commit 0c229a3 into main Aug 19, 2025
24 of 25 checks passed
@rdmark
rdmark deleted the null-pointer-abort branch August 19, 2025 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants