Skip to content
GitLab
  • Menu
Projects Groups Snippets
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in
  • Trac Trac
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Issues 246
    • Issues 246
    • List
    • Boards
    • Service Desk
    • Milestones
  • Monitor
    • Monitor
    • Metrics
    • Incidents
  • Analytics
    • Analytics
    • Value stream
  • Wiki
    • Wiki
  • Activity
  • Create a new issue
  • Issue Boards
Collapse sidebar
  • Legacy
  • TracTrac
  • Issues
  • #19531
Closed (moved) (moved)
Open
Created Jun 29, 2016 by David Goulet@dgoulet🔆

Major cleanup in our baseXX APIs

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);
  1. Source length requirement issue. I think though most of them are fixed except base64 API with ticket #17868 (moved).

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;
  1. 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.
  1. 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.

To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information
Assignee
Assign to
Time tracking