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

modules/resource-hwloc: Fix potential corner case #1012

Merged
merged 1 commit into from Mar 23, 2017

Conversation

Projects
None yet
5 participants
@chu11
Copy link
Contributor

chu11 commented Mar 22, 2017

Fix potential corner case if string buffer length includes
the NUL character at the end. We do not wish to include it
in the rpc. Instead, just write the string to the rpc directly
without the length (i.e. "{ s:s }" instead of "{ s:s# }").

Fixes #1010

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 22, 2017

Coverage Status

Coverage decreased (-0.004%) to 77.904% when pulling c7c9b26 on chu11:issue1010 into a549739 on flux-framework:master.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 22, 2017

Codecov Report

Merging #1012 into master will decrease coverage by 0.02%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1012      +/-   ##
==========================================
- Coverage   77.59%   77.57%   -0.03%     
==========================================
  Files         151      151              
  Lines       25776    25779       +3     
==========================================
- Hits        20002    19999       -3     
- Misses       5774     5780       +6
Impacted Files Coverage Δ
src/modules/resource-hwloc/resource.c 71.62% <66.66%> (-0.06%) ⬇️
src/common/libflux/rpc.c 90.44% <0%> (-0.74%) ⬇️
src/common/libflux/message.c 83.39% <0%> (-0.5%) ⬇️
src/common/libflux/handle.c 85.78% <0%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a549739...f6c04cb. Read the comment docs.

@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Mar 22, 2017

This is fine, but @chu11, in your opinion would it be safer to check first that buffer is NUL terminated before sending, just in case libhwloc ever changes to not return a NUL terminated string?

@chu11 chu11 force-pushed the chu11:issue1010 branch from c7c9b26 to 2541c76 Mar 22, 2017

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 22, 2017

@grondo Good idea!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 22, 2017

Coverage Status

Coverage decreased (-0.2%) to 77.736% when pulling 2541c76 on chu11:issue1010 into a549739 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 22, 2017

Huh ... it appears buffer[buflen - 1] != '\0' isn't always a safe check. Wondering what's going on.

2017-03-22T22:15:12.564860Z resource-hwloc.err[0]: hwloc_topology_export_xmlbuffer buffer not NUL terminated
flux-hwloc: flux_rpc_getf: Protocol error
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 22, 2017

Coverage Status

Coverage decreased (-0.2%) to 77.724% when pulling 4c0e826 on chu11:issue1010 into a549739 on flux-framework:master.

@chu11 chu11 force-pushed the chu11:issue1010 branch from 4c0e826 to 45facc4 Mar 22, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 22, 2017

Coverage Status

Coverage decreased (-0.2%) to 77.743% when pulling 45facc4 on chu11:issue1010 into a549739 on flux-framework:master.

@chu11 chu11 force-pushed the chu11:issue1010 branch from 45facc4 to f0a7f21 Mar 22, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 23, 2017

Coverage Status

Coverage decreased (-0.008%) to 77.899% when pulling f0a7f21 on chu11:issue1010 into a549739 on flux-framework:master.

@chu11 chu11 force-pushed the chu11:issue1010 branch from f0a7f21 to 49d52e5 Mar 23, 2017

modules/resource-hwloc: Fix potential corner case
The function hwloc_topology_export_xmlbuffer() appears
inconsistent in whether or not the returned buffer includes
the ending NUL char or not.  In addition, jansson library
appears inconsitent on whether buffer with buffer length
(i.e. 's#' argument) can contain NUL char.

Manipulate buflen argument to never contain ending NUL char
at end of string.

Fixes #1010

@chu11 chu11 force-pushed the chu11:issue1010 branch from 49d52e5 to f6c04cb Mar 23, 2017

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 23, 2017

Coverage Status

Coverage increased (+0.006%) to 77.914% when pulling f6c04cb on chu11:issue1010 into a549739 on flux-framework:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 23, 2017

Coverage Status

Coverage decreased (-0.02%) to 77.887% when pulling f6c04cb on chu11:issue1010 into a549739 on flux-framework:master.

@chu11

This comment has been minimized.

Copy link
Contributor Author

chu11 commented Mar 23, 2017

code coverage looks good, misses just one branch case (which is the case hit on opal).

@garlick

This comment has been minimized.

Copy link
Member

garlick commented Mar 23, 2017

Thanks! Merging.

@garlick garlick merged commit f78422e into flux-framework:master Mar 23, 2017

2 of 4 checks passed

codecov/patch 66.66% of diff hit (target 77.59%)
Details
codecov/project 77.57% (-0.03%) compared to a549739
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 77.887%
Details

@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.