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

Jump to bottom

Ticket28913: Make base32_decode() return number of bytes written. #611

Merged
merged 3 commits into from Feb 26, 2019
Merged

Ticket28913: Make base32_decode() return number of bytes written. #611

merged 3 commits into from Feb 26, 2019

Conversation

Reviewers

@dgoulet-tor dgoulet-tor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@nmathewson @coveralls @dgoulet-tor @torproject-pusher
@nmathewson
Copy link
Contributor

@nmathewson nmathewson commented Dec 20, 2018

No description provided.

nmathewson added 2 commits Dec 20, 2018
@nmathewson
base32_decode(): Return number of bytes written on success.
a517daa 
This makes it consistent with base64_decode().

Closes ticket 28913.
@nmathewson
Add base32 to the round-trip fuzzer
22c5ad6
@coveralls
Copy link

@coveralls coveralls commented Dec 20, 2018 •
edited

Pull Request Test Coverage Report for Build 3609

  • 6 of 11 (54.55%) changed or added relevant lines in 5 files are covered.
  • 7347 unchanged lines in 38 files lost coverage.
  • Overall coverage increased (+0.7%) to 61.74%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/feature/control/control.c 0 2 0.0%
src/feature/rend/rendparse.c 3 6 50.0%
Files with Coverage Reduction New Missed Lines %
src/feature/nodelist/routerlist.c 1 39.21%
src/lib/fs/freespace.c 1 77.78%
src/feature/rend/rendcommon.c 1 56.06%
src/feature/hs/hs_service.c 1 73.85%
src/feature/rend/rendcache.c 1 93.02%
src/feature/nodelist/nodelist.c 2 71.88%
src/core/or/versions.c 2 94.47%
src/feature/stats/rephist.c 2 40.49%
src/lib/time/tvdiff.c 3 90.0%
src/lib/crypt_ops/crypto_dh_openssl.c 4 97.12%
Totals Coverage Status
Change from base Build 3356: 0.7%
Covered Lines: 45249
Relevant Lines: 73290
💛 - Coveralls

dgoulet-tor
dgoulet-tor reviewed Dec 20, 2018
View changes
src/feature/rend/rendcache.c Outdated
@@ -854,7 +854,7 @@ rend_cache_store_v2_desc_as_client(const char *desc,
*entry = NULL;
}
if (base32_decode(want_desc_id, sizeof(want_desc_id),
desc_id_base32, strlen(desc_id_base32)) != 0) {
desc_id_base32, strlen(desc_id_base32)) < 0) {
Copy link
Contributor

@dgoulet-tor dgoulet-tor Dec 20, 2018

I think we should be looking at != REND_DESC_ID_V2_LEN_BASE32 here because with < 0 we could end up continuing with shorter or longer descriptor ID after which is undesirable I would say? (In reality, I don't think base32_decode can return a smaller size but for sake of correctness).

Copy link
Contributor

@dgoulet-tor dgoulet-tor Dec 20, 2018

If we go that way, this means that every places that does < 0 should then check for the expected length and not only an explicit error (-1). Decoding something that we expect 60 bit and getting 58 bit is imo an error.

Copy link
Contributor Author

@nmathewson nmathewson Dec 20, 2018

hm. that's probably right, but it's also a bunch more work. I don't suppose you'd like to write that patch maybe? ;) if not, I'll get to it when I can

@nmathewson
Fix users of base32_decode to check for expected length in return.
f632335 
Also, when we log about a failure from base32_decode(), we now
say that the length is wrong or that the characters were invalid:
previously we would just say that there were invalid characters.

Follow-up on 28913 work.
@torproject-pusher torproject-pusher merged commit f632335 into torproject:master Feb 26, 2019
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment