Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5346 closed defect (fixed)

parse_http_time is borked

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In his unit test adventures, emanchado found some problems with parse_http_time. Notably, the "Wednesday, 07 March 2012..." syntax was broken. Also, the months were all wrong when they got looked up by name.

I'm going to extract the patches to test and fix that into a separate branch so it can go on 0.2.2.x.

Child Tickets

Change History (12)

comment:1 Changed 8 years ago by nickm

Status: newneeds_review

See branch "bug5346" in my public repository.

comment:2 Changed 8 years ago by nickm

Status: needs_reviewneeds_revision

Review has turned up some other crazy things in our time parsing/handling.

wanoskarnet has found some places where we allow tm_sec to be up to 61. That's wrong; 60 is the highest supported value. We should grep for tm_sec to find all the cases of this.

Sebastian notes that some of our checks are based on incorrectly assuming tm_mon ranges from 1 to 12, rather than the correct 0..11. Further, tm_mday shouldn't be able to be 0 in a canonical date, but we seem to allow that.

comment:3 Changed 8 years ago by Sebastian

I also think we should do some gmtime stuff at the end of parse_http_time(), just to make sure we created a valid struct tm.

comment:4 Changed 8 years ago by nickm

Status: needs_revisionneeds_review

Pushed bug5346 again to fix those, and a ridiculous case where we were initializing hours=minutes=seconds=100 for no credible reason.

comment:5 Changed 8 years ago by Sebastian

you didn't fix

tm->tm_mon < 1
tm->tm_mon > 12

I'm making a branch here with a fix

comment:6 Changed 8 years ago by Sebastian

So, what about using gmtime? Also, in parse_http_time(), we don't set tm_wkday. I wonder if we should?

comment:7 Changed 8 years ago by nickm

Status: needs_reviewneeds_revision

I'm making a branch here with a fix

Not seeing the fix.

So, what about using gmtime?

It would be okay by me.

Also, in parse_http_time(), we don't set tm_wkday. I wonder if we should?

I think we're in the clear so long as we don't use it, and document that we don't set it.

comment:8 Changed 8 years ago by nickm

Status: needs_revisionneeds_review

Looking through all the code, I squashed it some and merged your tests. Right now, I'm happy to merge it as-is.

comment:9 Changed 8 years ago by nickm

(see my branch "bug5346_squashed")

comment:10 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Still looks okay to me. Merging into maint-0.2.2 and forward.

comment:11 Changed 7 years ago by nickm

Keywords: tor-client added

comment:12 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.