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

broker: avoid accidentally consuming % format characters in initial program args #2285

Merged
merged 4 commits into from Aug 3, 2019

Conversation

@grondo
Copy link
Contributor

commented Aug 3, 2019

Rename flux_cmd_argv_append() to flux_cmd_argv_appendf() to indicate this function takes printf style arguments. The new flux_cmd_argv_append() takes a string argument only, which satisfies most use cases and simultaneously fixes #2284, by avoiding passing user provided string as the fmt arg, as these strings may contain % characters.

This issue could have also been fixed by simply calling

 flux_cmd_argv_append (cmd, "%s", arg);

as one would do for printf, however it seemed important to try to avoid this confusion in the future, and perhaps the rename will help with that. (I'd be willing to redo the PR, though, if others feel different)

grondo added 2 commits Aug 3, 2019
Problem: Because the flux_cmd_argv_append() function takes printf
style format string and arguments, it suffers from the same problem as
printf(3) when a variable is passed directly to the fmt argument, as in

 flux_cmd_argv_append (cmd, arg);

since `arg` may contain a '%' character.

Possibly because of the function name, several spots in flux-core
call `flux_cmd_argv_append` just as shown above. This resulted in a
bug where '%h' and other characters preceeded by '%' can't be passed
through the `flux start` and `flux broker` command line.

To fix, rename `flux_cmd_argv_append` to `flux_cmd_argv_appendf` to
more strongly indicate that this is a printf style function. Add a new
`flux_cmd_argv_append()` which now takes a single string argument.

Update the few callers that needed the varargs `flux_cmd_argv_appendf`.

Fixes #2284
Add a small bit of coverage for flux_cmd_argv_appendf() to the
libsubprocess unit tests.
@garlick

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Maybe a __attribute__ ((format (printf, 2, 3))) would be useful here?

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Yeah, how did I forget about that attribute? I'll add that as well.

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Added __attribute__ ((format (printf, ...))) to flux_cmd_argv_appendf as well as flux_cmd_setenvf. There were some places where callers of flux_cmd_setenvf had to be fixed as well.

grondo added 2 commits Aug 3, 2019
Add __attribute__ ((format (printf, ...))) to flux_cmd_setenvf()
and flux_cmd_argv_appendf() to guard against callers calling these
functions with improper printf format style arguments.

This protects against bugs common with this style of arguments.

Fix a couple places in the code that were improperly calling these
functions, as caught by the new attribute.
Add a test that verifies printf format characters %h, %f, etc. may
be passed as expected on the broker command line to the initial
program.
@codecov-io

This comment has been minimized.

Copy link

commented Aug 3, 2019

Codecov Report

Merging #2285 into master will increase coverage by 0.03%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master    #2285      +/-   ##
==========================================
+ Coverage   80.79%   80.83%   +0.03%     
==========================================
  Files         213      213              
  Lines       33475    33484       +9     
==========================================
+ Hits        27047    27066      +19     
+ Misses       6428     6418      -10
Impacted Files Coverage Δ
src/cmd/builtin/proxy.c 74.41% <100%> (ø) ⬆️
src/broker/runlevel.c 81.5% <100%> (+0.1%) ⬆️
src/common/libsubprocess/server.c 71.7% <100%> (ø) ⬆️
src/modules/cron/task.c 79.31% <100%> (ø) ⬆️
src/modules/job-exec/exec.c 74.72% <100%> (ø) ⬆️
src/common/libsubprocess/command.c 70.38% <55.55%> (-0.49%) ⬇️
src/modules/connector-local/local.c 73.26% <0%> (+0.14%) ⬆️
src/common/libflux/reactor.c 92.08% <0%> (+0.22%) ⬆️
src/common/libflux/message.c 81% <0%> (+0.37%) ⬆️
src/common/libutil/veb.c 98.85% <0%> (+0.57%) ⬆️
... and 5 more
@garlick

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

LGTM! Ready to merge?

@grondo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

@garlick garlick merged commit 12facc9 into flux-framework:master Aug 3, 2019
3 of 4 checks passed
3 of 4 checks passed
codecov/patch 77.77% of diff hit (target 80.79%)
Details
Summary 1 potential rule
Details
codecov/project 80.83% (+0.03%) compared to 3b1fa35
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@garlick garlick referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.