[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #14013 [Core Tor/Tor]: base16_decode() API is inconsistent and error-prone
#14013: base16_decode() API is inconsistent and error-prone
-----------------------------------+------------------------------------
Reporter: nickm | Owner: nikkolasg
Type: defect | Status: merge_ready
Priority: High | Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor | Version:
Severity: Normal | Resolution:
Keywords: lorax, review-group-3 | Actual Points:
Parent ID: | Points: 1
Reviewer: dgoulet | Sponsor: SponsorS-can
-----------------------------------+------------------------------------
Changes (by dgoulet):
* status: needs_revision => merge_ready
Comment:
Replying to [comment:29 nickm]:
> requested changes:
> * require that destlen be less than SSIZE_MAX. Otherwise the cast in
base16_decode isn't safe.
Hrm actually that ssize_t cast doesn't make too much sense. That function
returns a int so we should align everything to `INT_MAX`. I originally
changed it to return `ssize_t` but I changed my mind so every
encode/decode function have the same interface.
Fixup commit: `b397307`
> * document what happens if destlen is greater than or less than
srclen/2.
Also in fixup commit: `b397307`
> *
>
> {{{
> + ok = base16_decode(id, DIGEST_LEN, cp+strlen("id="),
> + strlen(cp)-strlen("id=")) != DIGEST_LEN ? 0 :
1;
> }}}
>
> would make more sense as `ok = (base16_decode(...) == DIGEST_LEN)`
>
> * Why is the comparison in decode_hashed_passwords < rather than != ?
This was a leftover.
Both of the above fixed in fixup commit: `6bffbb9`
>
> To consider:
> * Should we make all of these functions clear the unused portion of
the output buffer?
Ideally, we should make all the decode function return how many bytes they
used so the caller can know the _exact_ amount filled up.
Now this patch makes it for base16 and base64 is already doing that.
base32 is the missing one. New ticket time I presume.
> * Is it possible that we missed any instances of base16_decode() ?
Always possible but nikkolasg originally made the patch and then I went
over each of the base16_decode I could find to fix some syntax and change
`<` to `!=`. So if we missed one, bad luck I would say.
See branch with the fixup commits: `bug14013_029_01`
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/14013#comment:30>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs