Opened 8 weeks ago

Closed 4 weeks ago

#31475 closed defect (fixed)

config: stop using atof()

Reported by: nickm Owned by: nickm
Priority: Low Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: asn-merge
Cc: Actual Points: .2
Parent ID: Points: 0.5
Reviewer: Sponsor:


The atof() call can't detect errors in a reasonable way, but we still use it when parsing configuration files. We should fix that.

Let's not touch this till after #29211 is done, since that code is in flux.

Child Tickets

Change History (11)

comment:1 Changed 6 weeks ago by nickm

Type: defectenhancement

Mark a number of current 0.4.2.x "defects" as "enhancements."

comment:2 Changed 5 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 5 weeks ago by nickm

Type: enhancementdefect

See branch ticket31475 with PR at .

I suggest no backport, even though this is (arguably) a bug: it's a behavior that nobody has noticed.

I'll put this in needs_review once CI has passed.

comment:4 Changed 5 weeks ago by nickm

Status: acceptedneeds_review

CI has passed

comment:5 Changed 5 weeks ago by dgoulet

Status: needs_reviewneeds_revision

Did a review and left a comment.

comment:6 Changed 5 weeks ago by nickm

I agree with your observation about overflow/underflow: I'll try to fix those.

I don't think that checking the return value for 0 is a good idea though, for two reasons:

  1. Our compilers give warnings when you do an == comparison with two doubles, so we'd have to work around that.
  2. My understanding is that the only way that *endptr can be equal to nptr is if no input is converted at all. If any input is converted, then *endptr points to the first unconverted byte.

comment:7 Changed 5 weeks ago by nickm

I've added overflow/underflow detection to the branch. I'm not checking for 0 or HUGE_VAL explicitly, but looking at the errno value instead. I've added tests to make sure it works.

comment:8 Changed 5 weeks ago by nickm

Actual Points: .2
Status: needs_revisionneeds_review

comment:9 Changed 5 weeks ago by dgoulet

Status: needs_reviewmerge_ready

lgtm thanks!

comment:10 Changed 5 weeks ago by nickm

Keywords: asn-merge added

comment:11 Changed 4 weeks ago by asn

Resolution: fixed
Status: merge_readyclosed


Note: See TracTickets for help on using tickets.