Opened 3 years ago

Closed 3 years ago

#19063 closed enhancement (implemented)

The tor_parse_* functions should check and warn on max < min

Reported by: teor Owned by: U+039b
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy
Cc: *@… Actual Points:
Parent ID: Points: small
Reviewer: Sponsor:

Description

If a developer mistakenly calls:

tor_parse_long(value, 10, 1, UINT32_MAX, NULL, NULL);

It effectively becomes:

tor_parse_long(value, 10, 1, -1, NULL, NULL);

We can detect this by making sure min <= max, and warning if that's not the case. (I really don't think we should assert.)
We should do this for all similar tor_parse_* functions.

But are there any circumstances where we should allow min to be greater than max? (it will always fail)
Existing callers pass constants to this function, so it's not going to trigger for them.

Child Tickets

Attachments (1)

0001-Fix-19063-Add-check-in-utility-macro.patch (1.7 KB) - added by U+039b 3 years ago.
Proposed patch

Download all attachments as: .zip

Change History (7)

comment:1 Changed 3 years ago by U+039b

Owner: set to U+039b
Status: newassigned

Changed 3 years ago by U+039b

Proposed patch

comment:2 Changed 3 years ago by U+039b

I propose the patch 0001-Fix-19063-Add-check-in-utility-macro.patch.

(max<min) is considered as an error:

  • add a check in the utility macro CHECK_STRTOX_RESULT.
  • add a test in test/utils.

No tor_parse_* call violate max>min and tests are ok.

comment:3 Changed 3 years ago by U+039b

Status: assignedneeds_review

comment:4 Changed 3 years ago by U+039b

Cc: *@… added

comment:5 Changed 3 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

lgtm; applied! (With a changes file)

comment:6 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed
Type: defectenhancement

Also, I wrapped the max < min test inside the BUG macro, so that we will can a warning and a stack trace if this ever occurs.

Note: See TracTickets for help on using tickets.