Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

search: Fix a NULL ptr deref with zero terms #4562

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

cgwalters
Copy link
Member

Here clang-analyzer found a legitimate bug, through a quite deep call stack. Before this change, providing zero search terms like this results in a segfault:

$ rpmostree_busctl_call_os Search as 0

In exactly the way predicted by the static analysis.

Verify we have at least one term at entry into the function, and also add further assertions later.

Here `clang-analyzer` found a legitimate bug, through a quite
deep call stack.  Before this change, providing zero search terms like
this results in a segfault:

```
$ rpmostree_busctl_call_os Search as 0
```

In exactly the way predicted by the static analysis.

Verify we have at least one term at entry into the function,
and also add further assertions later.
Copy link
Member

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cgwalters
Copy link
Member Author

/retest
Sigh, two different test flakes

@lukewarmtemp
Copy link
Contributor

lukewarmtemp commented Aug 29, 2023

@cgwalters Should I update the kola test with a case with zero search terms as well?

@cgwalters
Copy link
Member Author

@cgwalters Should I update the kola test with a case with zero search terms as well?

Yes, arguably I should have done that in this PR too. But we can do that in a followup PR to avoid a whole retest cycle.

Copy link
Contributor

@lukewarmtemp lukewarmtemp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think I implemented a check on the client side which prevents users from inputting zero packages when running rpm-ostree search, but completely forgot to consider the case when dbus is called directly, so thanks for catching this.

@cgwalters cgwalters merged commit f268221 into coreos:main Aug 29, 2023
13 checks passed
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.

3 participants