correct_tm() doesn't set r->tm_wday, but format_rfc1123_time() uses it

I think this is causing some unit tests to fail on Windows:

https://jenkins.torproject.org/job/tor-ci-mingwcross-0.2.8-test/ARCHITECTURE=i386,SUITE=jessie/9/console

changed milestone to %Tor: 0.2.7.x-final

added 025-backport 026-backport actualpoints::0.2 component::core tor/tor milestone::Tor: 0.2.7.x-final owner::nickm points::small priority::medium resolution::fixed reviewer::teor severity::normal status::closed type::defect version::tor 0.2.8.2-alpha labels

Minimal fix in branch bug18977_024.

Trac:
Keywords: 028-proposed deleted, 028-proposed 025-backport 026-backport 027-backport added

Trac:
Status: new to needs_review

Trac:
Owner: N/A to nickm
Status: needs_review to accepted

Code review:

T1: There are 4 cases, but only 3/4 set tm_wday.

How far we should backport:

This bug can crash on OSs where int t = 0; r = tor_gmtime_r(&t, ...); returns NULL, or a non-NULL r with r->tm_year outside the range -1899 to 8099. This has only been detected as a bug by our unit tests on Windows so far.

Once this case is triggered, if an int of uninitialised stack memory happens to be outside 0-6, tor crashes.

It's only triggered by code paths that set if_modified_since in directory_send_command() to 0, and most code paths on relays and clients set if_modified_since to 0. (It might also be possible for authorities to trigger this crash by passing a directory server a cached-but-not-validated consensus that has a bad time value, but I have yet to confirm this.)

There are many clients but few relays on Windows.

Given the simplicity of this patch, and the fact that a failure causes a crash, I suggest we backport it to 0.2.4 (the latest running version on the network) onwards.

Trac:
Status: accepted to needs_revision

Replying to teor:

Code review:

T1: There are 4 cases, but only 3/4 set tm_wday.

bug18977_024 now has a fixup commit, but I don't know if I got the case you meant. If I didn't, please let me know where it is?

[...]

Given the simplicity of this patch, and the fact that a failure causes a crash, I suggest we backport it to 0.2.4 (the latest running version on the network) onwards.

Okay. Let's try on 0.2.7+, and backport ad libitum.

Trac:
Status: needs_revision to needs_review

Trac:
Reviewer: N/A to teor

Looks good, but there's another case in correct_tm in master (maybe since 0.2.6 or 0.2.7?)

bug18977_024_v2 is as above, but squashed.

bug18977_026_v2 merges that to maint-0.2.6 , with the extra fix for the extra case.

Trac:
Actualpoints: N/A to 0.2

Thanks, both branches look good.

Trac:
Status: needs_review to merge_ready

Okay. I've merged to 0.2.7 and forward, and am marking for consideration for 0.2.6 backport.

Trac:
Keywords: 028-proposed 025-backport 026-backport 027-backport deleted, 025-backport 026-backport added
Status: merge_ready to needs_review
Milestone: Tor: 0.2.8.x-final to Tor: 0.2.6.x-final

No 0.2.6 backport.

Trac:
Milestone: Tor: 0.2.6.x-final to Tor: 0.2.7.x-final
Resolution: N/A to fixed
Status: needs_review to closed

closed

added 1h 36m of time spent

moved to tpo/core/tor#18977 (closed)