Opened 4 years ago

Closed 4 years ago

#15652 closed enhancement (implemented)

Base64 code cleanups.

Reported by: yawning Owned by: yawning
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: tor-core
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The base64 encoder/decoder can use some love of the following sort:

  • The code gated by USE_OPENSSL_BASE64 should probably be removed, since we haven't used OpenSSL's base64 decoder since 2007, and enabling it will do weird and broken things (Eg: Inputs that aren't broken up into into lines that are < 80 characters will fail to decode correctly).
  • It would be really nice to have a Base64 encode that doesn't automatically add \n characters.
  • Having a routine to get the encoded output size makes for cleaner code.

Child Tickets

Change History (4)

comment:1 Changed 4 years ago by yawning

Status: newneeds_review

https://github.com/Yawning/tor/compare/feature15652

Both output formats should have a reasonable amount of test coverage. If it needs to be faster, it can be for both output format cases, but it would complicate the code somewhat.

comment:2 Changed 4 years ago by nickm

ba2485f7df51b2daafaff8567320c34a22731e8e

  • I like.

e58d27f186ec5a8f67b4aee4513c3e02ff7f29b9

  • I think that ENCODE_OPENSSL is maybe not so great a name. Maybe ENCODE_MULTILINE?
  • Style says to wrap macros in do { ... } while (0). Harmless in this case.
  • I wonder whether it's better to have the new behavior in a new function, so that less of the code needs to change. But I'd say that it's our call in this case though.

We should have an additional patch after this series that moves base{64,32}_encode/decode out of crypto into util or something. Doesn't need to be now though.

comment:3 in reply to:  2 Changed 4 years ago by yawning

Replying to nickm:

e58d27f186ec5a8f67b4aee4513c3e02ff7f29b9

  • I think that ENCODE_OPENSSL is maybe not so great a name. Maybe ENCODE_MULTILINE?

1100a83ce45d1872be8333bea1a1e55dd318dfe7

  • Style says to wrap macros in do { ... } while (0). Harmless in this case.

0b8bb8988f380bf2c9c3ffc3b21c7457db239813

  • I wonder whether it's better to have the new behavior in a new function, so that less of the code needs to change. But I'd say that it's our call in this case though.

If anything I would be tempted to put the old behavior in a function that explicitly adds multiline output since that's a PEM-ism (and OpenSSL's PEM output flavor as well). I think what's in the branch is clear enough though.

I amended a commit so I can give the autosquash thing a whirl, you might need to re-pull. (There's also one more commit past what you've seen that fixes up a doxygen comment).

comment:4 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks good now. Merged, along with 55118d90ca927d2cdb2cbd92189113da4dcb2fe7 to fix some implicit conversion warnings.

Note: See TracTickets for help on using tickets.