#29960 closed enhancement (fixed)

Actually check for errors in digest256_to_base64() and callers

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.2.2.6-alpha
Severity: Normal Keywords: asn-merge, technical-debt, fast-fix
Cc: Actual Points: 0.4
Parent ID: Points: 0.2
Reviewer: nickm Sponsor: Sponsor31-can

Description

We don't propagate errors from base64_encode() to digest256_to_base64() and its callers.

Discovered as part of #29959.

Child Tickets

Change History (9)

comment:1 Changed 18 months ago by teor

Status: assignedneeds_review
Version: Tor: 0.2.2.6-alpha

comment:2 Changed 18 months ago by asn

Reviewer: nickm

comment:3 Changed 18 months ago by nickm

Status: needs_reviewneeds_revision

a76e833 probably belongs to another branch?

Looking at this code, I think we might be better off changing the API here so that digest256_to_base64() is documented as "cannot fail", and returns void. It would simplify all of its callers and their callers in turn, and more accurately reflect reality.

After all, the only circumstances where it can fail would be if base64_encode(), when given a 256-bit input, gives a truncated output. But I believe that's not possible, unless our base64_encode() implementation is broken.

comment:4 Changed 18 months ago by teor

https://github.com/torproject/tor/pull/888 in #29959 removes the check in format_networkstatus_vote(), so I should avoid touching those lines in my revised patch.

comment:5 Changed 18 months ago by teor

Actual Points: 0.20.4
Status: needs_revisionneeds_review
Type: defectenhancement

I re-did the PR, with asserts for impossible failures. I also ran it through chutney, in case I made any mistakes.

Please review https://github.com/torproject/tor/pull/908

comment:6 Changed 18 months ago by nickm

Status: needs_reviewneeds_revision

Looks good. I've noted that a use of 'buf' in ed25519_signature_from_base64 is no longer needed. If you agree, please feel free to add an additional commit and put this in merge_ready.

comment:7 Changed 18 months ago by teor

Status: needs_revisionmerge_ready

I agree, added a commit to remove it.

comment:8 Changed 18 months ago by teor

Keywords: asn-merge added

comment:9 Changed 18 months ago by asn

Resolution: fixed
Status: merge_readyclosed

merged to master.

Note: See TracTickets for help on using tickets.