[tor-bugs] #19531 [Core Tor/Tor]: Major cleanup in our baseXX APIs

[Tor Bug Tracker & Wiki]

#19531: Major cleanup in our baseXX APIs
------------------------------+-------------------------------
     Reporter:  dgoulet       |      Owner:
         Type:  enhancement   |     Status:  new
     Priority:  Medium        |  Milestone:  Tor: 0.2.???
    Component:  Core Tor/Tor  |    Version:
     Severity:  Normal        |   Keywords:  029-proposed util
Actual Points:                |  Parent ID:
       Points:  1             |   Reviewer:
      Sponsor:                |
------------------------------+-------------------------------
 Our base16/32/64 APIs are quite inconsistent and a timebomb of possible
 errors and issues. Here is some recommendation with this:

 1) We should have for _each_ type of encoding a function that returns the
 encoded size using a source length. We have such function for baese32 and
 base64 but they are not consistent:

 - base32: `base32_encoded_size()` returns the size including the NUL byte

 - base64: `base64_encode_size()` has a different name and does NOT add the
 NUL byte.

 They should all return the size with the extra NUL byte because every
 `_encode()` function we have requires it in the first place. The other
 solution is to have a new function `baseXX_encoded_string_size()` or
 something that return the NUL byte and the non string function returns the
 value without it.

 Else we end up with this kind of code pattern:
 {{{
 len = base64_encoded_size() + 1
 buf = tor_malloc_zero(len)
 ret = base64_encode()
 tor_assert(ret == len - 1)
 }}}

 or the +1 added explicitly like so which is _not_ good.
 {{{
 base32_encode(serviceid, REND_SERVICE_ID_LEN_BASE32+1,
               circuit->rend_data->rend_pk_digest, REND_SERVICE_ID_LEN);
 }}}

 or even better:
 {{{
 base16_encode(hex, 2*CONDITIONAL_CONSENSUS_FPR_LEN+1,
               ds->v3_identity_digest, CONDITIONAL_CONSENSUS_FPR_LEN);
 }}}

 2) Source length requirement issue. I think though most of them are fixed
 except base64 API with ticket #17868.

 I do see this in the `base16_decode` which could either be a _hard_
 requirement assert or assume that if it gets 9 bytes in srclen, well only
 8 are decoded. We have callsites that do NOT check the returned value
 so...

 {{{
   if ((srclen % 2) != 0)
     return -1;
 }}}

 3) Every baseXX_encode/decode should return the amount of bytes that has
 been encoded or decoded. They also should return `ssize_t;` but that's
 another ticket I feel like because it's a large refactoring. Here are the
 missing pieces:

 - `base16_encode()` returns void so it should return `int` as the encoded
 length.
 - `base32_encode()` returns void.
 - `base32_decode()` returns 0 on success.

 4) We should also have macros for the encoded length computation else we
 ended up with stuff like this (taken from the prop250 branch).

 {{{
 #define SR_SRV_VALUE_BASE64_LEN \
   (((DIGEST256_LEN - 1) / 3) * 4 + 4)
 }}}

 or flat values

 {{{
 #define REND_DESC_COOKIE_LEN_BASE64 22
 }}}

 This is a static calculation so most of the times that macro would be more
 useful than the function since it could be used for stack allocation which
 is a BIG use case of ours.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/19531>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online