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 liboptparse seg-fault corner case #927

Merged
merged 3 commits into from Dec 14, 2016

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Dec 14, 2016

No description provided.

liboptparse: Fix potential segfault
Error in call to optstring_append() can lead to NULL pointer
dereference later in the option_table_create() function.

@garlick garlick added the review label Dec 14, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 14, 2016

Coverage Status

Coverage decreased (-0.01%) to 76.299% when pulling ffaecee on chu11:liboptparsesegfault into e84a5b5 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 14, 2016

Current coverage is 75.99% (diff: 71.42%)

Merging #927 into master will decrease coverage by 0.01%

@@             master       #927   diff @@
==========================================
  Files           149        149          
  Lines         26001      26006     +5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          19763      19762     -1   
- Misses         6238       6244     +6   
  Partials          0          0          
Diff Coverage File Path
••••••• 71% src/common/liboptparse/optparse.c

Powered by Codecov. Last update e84a5b5...40cb887

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Dec 14, 2016

code coverage appears to be fine, the missed lines are the error handling from a failed allocation using strdup, which is of course hard to capture in testing.

memset (&opts[j], 0, sizeof (struct option));

/* Corner case, no options configured by user */
if (sp && !(*sp)) {

This comment has been minimized.

@grondo

grondo Dec 14, 2016

Contributor

This is a good fix, Thanks! However, handling the case down here took me a minute to grok, and seems a bit out of place after the fact. Would it work if we initialized *sp with optstring_create() at the top of the function? Then it is just "proper initialization" and we don't have to handle no options as a corner case (though optstring_create() makes the string as "+" so we'd have to see if that is valid input to getopt_long...)

This comment has been minimized.

@chu11

chu11 Dec 14, 2016

Author Contributor

Ahh, you're right, that's a much better and cleaner fix. I'll do a push.

chu11 added some commits Dec 13, 2016

liboptparse: Fix segfault corner case
Fix segfault corner case in which user does not desire any
options.
test/optparse: Add corner case test
Add regression test for segfault corner case.

@chu11 chu11 force-pushed the chu11:liboptparsesegfault branch from ffaecee to 40cb887 Dec 14, 2016

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 14, 2016

Coverage Status

Coverage decreased (-0.01%) to 76.298% when pulling 40cb887 on chu11:liboptparsesegfault into e84a5b5 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Dec 14, 2016

Nice! and thanks for the new testcase!

@grondo grondo merged commit 82e5275 into flux-framework:master Dec 14, 2016

2 of 4 checks passed

codecov/patch 71.42% of diff hit (target 76.00%)
Details
codecov/project 75.99% (-0.02%) compared to e84a5b5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 76.298%
Details

@grondo grondo removed the review label Dec 14, 2016

@grondo grondo referenced this pull request Mar 28, 2017

Closed

0.7.0 Release Notes #1019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.