Opened 3 years ago

Closed 2 years ago

#21872 closed enhancement (fixed)

encoded length macros for baseXX encodings

Reported by: catalyst Owned by: catalyst
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: util
Cc: Actual Points:
Parent ID: #19531 Points:
Reviewer: Sponsor:

Description

It can be useful to have macros that give the lengths of byte strings encoded with baseXX, particularly for declaring fixed-size buffers. Some use cases will need to include a terminating NUL, others not. These macros won't check for overflow, so nothing should pass untrusted inputs to them.

I propose macros named like BASE64_LEN() for the raw length and like BASE64_SIZE() for the length with NUL.

Child Tickets

Change History (12)

comment:1 Changed 3 years ago by nickm

I like this idea, except that SIZE vs LEN seems a bit error-prone. how about ALLOC_SIZE instead of SIZE()?

comment:2 Changed 3 years ago by catalyst

That's a good point. I'd like to keep the macro names short, especially if they're going to end up in array declarations. Maybe something like BASEXX_BUFSIZE()?

comment:3 Changed 3 years ago by nickm

BUFSIZE sounds good

comment:5 Changed 3 years ago by dgoulet

Status: newneeds_review

comment:6 Changed 3 years ago by dgoulet

This looks good to me!

Quick question, do we have unit tests for those baseXX APIs? I'm asking because some tor_assert() are being added like tor_assert(destlen >= BASE16_BUFSIZE(srclen)); which look safe but still useful to make sure tor code will not blow up there :).

comment:7 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Added tests for basexx size apis in 30b13fd82e243713c6a0d7bfbfc97d5faf8ee9c8.

Merged; tests were okay.

Added an additional warning in the comments in 1e54bdd48a5deec7c1c4a0a167fb5abe312c8458.

Pushed to master.

Thanks!

comment:8 Changed 2 years ago by Jigsaw52

Resolution: implemented
Status: closedreopened

I found a bug on these macro definitions while working on #7869:

If you look closely at the following macros:

#define BASE64_NOPAD_LEN(n) (CEIL_DIV((n) * 4, 3)
#define BASE32_NOPAD_LEN(n) (CEIL_DIV((n) * 8, 5)

#define BASE64_NOPAD_BUFSIZE(n) (BASE64_NOPAD_LEN(n) + 1))
#define BASE32_NOPAD_BUFSIZE(n) (BASE32_NOPAD_LEN(n) + 1))

You will notice that BASE64_NOPAD_LEN and BASE32_NOPAD_LEN are missing the closing parentheses.
Also, BASE64_NOPAD_BUFSIZE and BASE32_NOPAD_BUFSIZE have one extra closing parentheses.
If you use only BASE64_NOPAD_BUFSIZE and BASE32_NOPAD_BUFSIZE everything will be fine as both errors cancel each other out.

Here is my fix: https://github.com/Jigsaw52/tor/tree/baseXX-macro-fix-21872

comment:9 Changed 2 years ago by arma

Good find!

comment:10 Changed 2 years ago by catalyst

Thanks for finding that! I'll probably cherry pick the macro fix individually and then put the rest of your #7869 patch in a separate commit, unless you want to split the commits yourself.

comment:11 Changed 2 years ago by catalyst

Status: reopenedmerge_ready

Oops, missed that you already had a branch with only the macro fixes in it. Also I'm very amused at the bugs canceling each other out.

Looks good to me; setting to merge_ready.

comment:12 Changed 2 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged it; thanks!

Note: See TracTickets for help on using tickets.