Opened 4 months ago

Closed 8 weeks ago

#22802 closed defect (fixed)

Avoid use of "0" with tor_parse_foo()

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-22
Cc: catayst Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The tor_parse_long() functions and family can behave a little different on different platforms when the underlying strto* functions behave differently. This is exacerbated by use of bases other than 10, as we found on #22789. Let's stop using "base 0" (the 'autodetect' base) anywhere in our code.

Child Tickets

Change History (9)

comment:1 Changed 4 months ago by teor

In #22469, tor accepts 0x00 as a port number in the torrc, despite it being against the spec. I wonder if we do the same thing when parsing ports in other contexts?

comment:2 in reply to:  1 ; Changed 2 months ago by nickm

Replying to teor:

In #22469, tor accepts 0x00 as a port number in the torrc, despite it being against the spec. I wonder if we do the same thing when parsing ports in other contexts?

I'm not sure we actually do that here -- looking over the tor_parse_* functions, I couldn't find any that would have caused #22469.

comment:3 Changed 2 months ago by nickm

Status: newneeds_review

I've fixed the cases I could see in bug22802.

I also cleaned up a bogus case in the windows tor_parse_uint64() implementation while I was there.

comment:4 in reply to:  2 Changed 2 months ago by catalyst

Cc: catayst added

Replying to nickm:

I'm not sure we actually do that here -- looking over the tor_parse_* functions, I couldn't find any that would have caused #22469.

Commented on #22469 about a possible cause.

comment:5 Changed 2 months ago by nickm

Owner: set to nickm
Status: needs_reviewassigned

setting owner

comment:6 Changed 2 months ago by nickm

Status: assignedneeds_review

comment:7 Changed 2 months ago by nickm

Keywords: review-group-22 added

comment:8 Changed 8 weeks ago by asn

Status: needs_reviewneeds_revision

Seems like we added a forgotten

+  // ????

in parse_process_specifier().

Also if we are serious about this, should we add an tor_assert_nonfatal(base > 0) in tor_parse_long() to make sure that we don't do this mistake in the future?

Other than that, patch looks good to me!

comment:9 Changed 8 weeks ago by nickm

Resolution: fixed
Status: needs_revisionclosed

I've removed the ????, squashed, and merged.

I'm not sure about the base>0 check; it's not _always_ wrong -- it's just wrong for inputs that we expect to be in decimal. I'm planning to leave it as-is.

Note: See TracTickets for help on using tickets.