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 #821: crash in kvs due to NULL arg in Jget_str() #822

Merged
merged 3 commits into from Sep 25, 2016

Conversation

Projects
None yet
3 participants
@grondo
Copy link
Contributor

grondo commented Sep 25, 2016

Fix #821 and possible similar bugs by protecting most return by reference calls in shortjson.h from NULL args.

Also, add a regression test to the issues test driver for the kvs pattern that initially triggered the bug.

grondo added some commits Sep 25, 2016

libutil/shortjson.h: protect against NULL arguments
In shortjson.h, protect against "return by reference" arguments that
may be NULL. This should make code more robust, and allows code to
check for existence of an entry in a JSON object without assigning
it to a variable.

Fixes #821
t/issues: add t0821-kvs-segfault.sh
Add regression test for issue #821, kvs segfault during get
of subdirectory of a FILREF.
build: add t/issues/t0821-kvs-segfault.sh
Add t0821-kvs-segfault.sh to dist

@grondo grondo added the review label Sep 25, 2016

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 25, 2016

Perfect! I'll push the button once travis goes green.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 25, 2016

Coverage Status

Coverage decreased (-0.06%) to 75.236% when pulling 21aab46 on grondo:issue#821 into 400a743 on flux-framework:master.

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Sep 25, 2016

Hm, clang checks failed here:

not ok 71 - stat watcher invoked once for size chnage
#   Failed test 'stat watcher invoked once for size chnage'
#   at ../../../../src/common/libflux/test/reactor.c line 644.

Which I think we saw also when testing python changes. Seems like maybe an intermittent test failure under travis has popped up.

I'll try restarting the test this time.

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Sep 25, 2016

That's got to be unrelated, so merging. Thanks for fixing my bugs!

@garlick garlick merged commit bc651df into flux-framework:master Sep 25, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.06%) to 75.236%
Details

@garlick garlick removed the review label Sep 25, 2016

@grondo

This comment has been minimized.

Copy link
Contributor Author

grondo commented Sep 25, 2016

Thanks!

On Sep 25, 2016 11:31 AM, "Jim Garlick" notifications@github.com wrote:

Merged #822 #822.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#822 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAtSUus7OT-MVYJ7ZXdMfywb1VmzSQ9gks5qtr4agaJpZM4KF8sI
.

@grondo grondo deleted the grondo:issue#821 branch Oct 22, 2016

@garlick garlick referenced this pull request Oct 26, 2016

Closed

0.5.0 release notes #879

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.