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

base64: switch to libsodium implementation #1786

Merged
merged 14 commits into from Oct 31, 2018

Conversation

Projects
None yet
5 participants
@garlick
Copy link
Member

garlick commented Oct 31, 2018

This PR switches users of the base64 implementation that we borrowed from munge over to the one in libsodium per discussion in #1775.

One thing that's slightly different about the libsodium implementation is that on decoding (base64 to bin), it does not implicitly pad with a \0 byte. The munge version did this, and did not include it in the length. We were relying on that in one spot in libkvs/treeobj.c. I will open a bug on fixing our reliance on that (did not want to get distracted with it here).

Another difference is that there is no function in libsodium for computing the decoded size in advance so the exact buffer size can be allocated. I used a value of ((srclen + 3) / 4) * 3 (math borrowed from the munge base64.c, without its +1 for the aforementioned \0).

Finally error handling is a bit different. The encode function always succeeds, which is expected since the buffer is pre-allocated of a size computed by libsodium. If you manage to pass a buffer that is too small, then libsodium internally calls its sodium_misuse() function, which calls abort().

Docs and configure are updated for the new external dependency. The Docker builds for travis were already installing it.

Fixes #1775

garlick added some commits Oct 30, 2018

build: require libsodium >= 1.0.14
libsodium provides a base64 implementation that can
replace libutil/base64.[ch].  It was added in version
1.0.14.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 31, 2018

Codecov Report

Merging #1786 into master will decrease coverage by 0.07%.
The diff coverage is 76.59%.

@@            Coverage Diff             @@
##           master    #1786      +/-   ##
==========================================
- Coverage   79.66%   79.58%   -0.08%     
==========================================
  Files         187      186       -1     
  Lines       34724    34560     -164     
==========================================
- Hits        27662    27504     -158     
+ Misses       7062     7056       -6
Impacted Files Coverage Δ
src/modules/kvs/kvs.c 65.55% <ø> (ø) ⬆️
src/common/libflux/security.c 76.47% <ø> (ø) ⬆️
src/common/libsubprocess/local.c 76.27% <ø> (ø) ⬆️
src/broker/exec.c 65% <ø> (ø) ⬆️
src/common/libsubprocess/util.c 92.3% <ø> (ø) ⬆️
src/common/libsubprocess/subprocess.c 87.55% <ø> (ø) ⬆️
src/common/libflux/event.c 80.14% <100%> (+0.56%) ⬆️
src/common/libzio/zio.c 74.5% <100%> (+0.32%) ⬆️
src/broker/publisher.c 82.47% <100%> (ø) ⬆️
src/common/libjob/sign_none.c 93.75% <100%> (-0.23%) ⬇️
... and 8 more
@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 31, 2018

would it be worthwhile putting the (((X + 3) / 4) * 3) into some macro somewhere? I'd hate if a new file has to be created, but it does seem cut & pasted a lot.

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 31, 2018

Yeah... thoughts on where? Is that appropriate for libutil/macros.h? E.g.

// maximum size of buffer needed to decode base64 string of length 'x'
#define BASE64_DECODE_SIZE(x) ((((x) + 3) / 4) * 3)
@grondo

This comment has been minimized.

Copy link
Contributor

grondo commented Oct 31, 2018

Is that appropriate for libutil/macros.h?

Seems like as good a place as any to me.

You might want to keep @dun's nice explanation from the original source (and also make a note about not including space for terminating NUL)

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 31, 2018

Ah this

/*  When decoding, 4 characters are decoded into 3 bytes.
 *  Add 3 bytes to ensure a partial 4-byte chunk will be accounted for
 *    during integer division, then add 1 byte for the terminating NUL.
 */

Yes, I'll add something to that effect.

@garlick garlick force-pushed the garlick:base64_sodium branch from 544b6b6 to 6036bf6 Oct 31, 2018

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 31, 2018

OK I made that change and forced a push. Macro is documented as follows. @dun, I pilfered some of your words, hope you don't mind.

/* Maximum size of buffer needed to decode a base64 string of length 'x',
 * where 4 characters are decoded into 3 bytes.  Add 3 bytes to ensure a
 * partial 4-byte chunk will be accounted for during integer division.
 * This size is safe for use with all (4) libsodium base64 variants.
 * N.B. unlike @dun's base64 implementation from the munge project,
 * this size does not include room for a \0 string terminator.
 */
#define BASE64_DECODE_SIZE(x) ((((x) + 3) / 4) * 3)
@dun

This comment has been minimized.

Copy link

dun commented Oct 31, 2018

@garlick Looks good to me. 👻

@chu11
Copy link
Contributor

chu11 left a comment

LGTM, just noticed one nit.

@@ -587,7 +588,7 @@ static int remote_output (flux_subprocess_t *p, flux_future_t *f,

if (tmp != len) {
flux_log_error (p->h, "channel buffer error: rank = %d pid = %d, stream = %s, len = %d",
rank, pid, stream, len);
rank, pid, stream, (int)len);

This comment has been minimized.

@chu11

chu11 Oct 31, 2018

Contributor

instead of casting, perhaps should just use %zu (i think) specifier?

This comment has been minimized.

@garlick

garlick Oct 31, 2018

Author Member

Ok, fixed and forced a push.

@@ -427,7 +429,7 @@ static int write_subprocess (flux_subprocess_server_t *s, flux_subprocess_t *p,

if (tmp != len) {
flux_log_error (s->h, "channel buffer error: rank = %d pid = %d, stream = %s, len = %d",
s->rank, flux_subprocess_pid (p), name, len);
s->rank, flux_subprocess_pid (p), name, (int)len);

This comment has been minimized.

@chu11

chu11 Oct 31, 2018

Contributor

likewise here.

This comment has been minimized.

@garlick

garlick Oct 31, 2018

Author Member

likewise, thanks.

@garlick garlick force-pushed the garlick:base64_sodium branch from 6036bf6 to 6ecba49 Oct 31, 2018

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 31, 2018

I think this is ok to merge, although I'm unsure why codecov says "CI" failed? diff coverage seems acceptable, as some error paths weren't hit. Should it just be re-run?

@garlick

This comment has been minimized.

Copy link
Member Author

garlick commented Oct 31, 2018

Hmm that is odd. I did restart a builder that had hung without much to go on. about a half hour ago.

Coverage was OK before those minor changes so prob its OK to merge (IMHO)

@chu11

This comment has been minimized.

Copy link
Contributor

chu11 commented Oct 31, 2018

oh that's weird, the "diff coverage" stat also disappeared, it was like 76% out of 79% a minute ago. codecov's twitter says there's some issues, so I'll assume that's just it. Hitting the button.

@chu11 chu11 merged commit 31a60be into flux-framework:master Oct 31, 2018

1 of 3 checks passed

codecov/patch CI failed.
Details
codecov/project CI failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.