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

Fix crash when running with --list-listeners and no registered listeners #2442

Merged
merged 2 commits into from
Jun 2, 2022

Conversation

loximann
Copy link
Contributor

@loximann loximann commented Jun 2, 2022

Description

Running with --listeners without registered listeners caused Catch2 programs to crash.

$ ./a.out --list-listeners
zsh: segmentation fault (core dumped)  ./a.out --list-listeners

The reason was due to dereferencing an iterator pointing past the end of a vector when the vector is empty.

@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #2442 (7ae9ffc) into devel (cca5923) will decrease coverage by 0.01%.
The diff coverage is 66.67%.

❗ Current head 7ae9ffc differs from pull request most recent head 0057eb1. Consider uploading reports for the commit 0057eb1 to get more accurate results

@@            Coverage Diff             @@
##            devel    #2442      +/-   ##
==========================================
- Coverage   91.43%   91.42%   -0.01%     
==========================================
  Files         159      159              
  Lines        7501     7503       +2     
==========================================
+ Hits         6858     6859       +1     
- Misses        643      644       +1     

@loximann
Copy link
Contributor Author

loximann commented Jun 2, 2022

Should I add a test case to cover listing when no listeners are registered?

@horenmar
Copy link
Member

horenmar commented Jun 2, 2022

Should I add a test case to cover listing when no listeners are registered?

Yes.

@horenmar horenmar added the BugFix label Jun 2, 2022
@loximann loximann force-pushed the fix_list_listeners_crash branch from 7ae9ffc to 9bf2eae Compare June 2, 2022 11:47
@loximann
Copy link
Contributor Author

loximann commented Jun 2, 2022

I modified the fix so that defaultListListeners is single return (which in general I think makes things cleaner). With that, no test cases are needed for full coverage; however I added a test case to avoid regressions.

@horenmar
Copy link
Member

horenmar commented Jun 2, 2022

Please keep the early exit.

And coverage isn't really important, the regression check is.

@loximann loximann force-pushed the fix_list_listeners_crash branch from 9bf2eae to 0057eb1 Compare June 2, 2022 11:58
@horenmar horenmar merged commit 5efd327 into catchorg:devel Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants