Opened 3 months ago

Closed 4 days ago

#28614 closed defect (fixed)

Can't parse networkstatus consensus time

Reported by: Vort Owned by: nickm
Priority: High Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 040-rc-must, regression, postfreeze-ok, tbb-needs
Cc: cypherpunks2 Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor:

Description

When I run tor.exe -f torrc, one of the first messages in log file is:
Nov 25 19:51:48.000 [warn] ISO time "2018-11-25 17:00:00\r" was unparseable
Nov 25 19:51:48.000 [warn] Unable to parse networkstatus consensus

Version tested: 3741f9e5 on Windows 7 SP1 x64.

Child Tickets

Attachments (2)

tor_cons.png (23.3 KB) - added by Vort 3 months ago.
Screenshot
tor_mmap.png (130.0 KB) - added by Vort 2 months ago.
Stack trace

Download all attachments as: .zip

Change History (29)

comment:1 Changed 3 months ago by nickm

Milestone: Tor: 0.4.0.x-final

Hm. Does this happen with older versions of Tor, or is it a regression with this one?

To me it sounds like the issue is with the \r at the end of the line -- like the consensus file was saved in a windows text format, but it's being interpreted as a unix-style text format.

comment:2 Changed 3 months ago by Vort

Never seen this with 0.3.4.9.

comment:3 Changed 3 months ago by gk

Version: Tor: 0.3.5.5-alpha

I was about to file this bug, too. This happens on Windows as well when testing in a Tor Browser context. I think this prevents Tor Browser starting up properly as I've seen numerous instances where Tor Browser "hangs" for 20 minutes after those messages. But that's "only" happening with a clean, new bundle on first start. Luckily, 0.3.5.5-alpha is fine, though.

comment:4 Changed 3 months ago by teor

Keywords: 040-rc-must regression added

comment:5 Changed 3 months ago by nickm

Is there a way to reproduce this easily? Is there a Tor version where it always happens, and one where it never happens?

I wonder if the mmap-related changes of #27244 could be responsible here.

Changed 3 months ago by Vort

Attachment: tor_cons.png added

Screenshot

comment:6 Changed 3 months ago by Vort

Reproduction is simple: just run tor.exe.

Changed 2 months ago by Vort

Attachment: tor_mmap.png added

Stack trace

comment:7 Changed 2 months ago by Vort

Yes, it may be related to mmap.
Here is additional info: tor_mmap.png.

comment:8 in reply to:  3 ; Changed 2 months ago by gk

Replying to gk:

I was about to file this bug, too. This happens on Windows as well when testing in a Tor Browser context. I think this prevents Tor Browser starting up properly as I've seen numerous instances where Tor Browser "hangs" for 20 minutes after those messages. But that's "only" happening with a clean, new bundle on first start. Luckily, 0.3.5.5-alpha is fine, though.

I think I am wrong. This just happens with testing Tor Browser 8.0.4 which ships 0.3.4.9. The result is that bootstrapping is delayed for several minutes (assuming that delay is actually caused by that parsing issue). I wonder, though, while this seems to be a new bug...

comment:9 in reply to:  8 Changed 2 months ago by gk

Replying to gk:

Replying to gk:

I was about to file this bug, too. This happens on Windows as well when testing in a Tor Browser context. I think this prevents Tor Browser starting up properly as I've seen numerous instances where Tor Browser "hangs" for 20 minutes after those messages. But that's "only" happening with a clean, new bundle on first start. Luckily, 0.3.5.5-alpha is fine, though.

I think I am wrong. This just happens with testing Tor Browser 8.0.4 which ships 0.3.4.9. The result is that bootstrapping is delayed for several minutes (assuming that delay is actually caused by that parsing issue). I wonder, though, while this seems to be a new bug...

Wait, scratch that. I think I took the wrong bundle to test. Sorry for the noise.

comment:10 Changed 2 months ago by gk

Note: #28405 seems highly related (but does have a different milestone and keywords).

comment:11 Changed 5 weeks ago by nickm

Keywords: postfreeze-ok added

Mark some tickets as postfreeze-ok, to indicate that I think they are okay to accept in 0.4.0 post-freeze. Does not indicate that they are all necessary to do postfreeze.

comment:12 Changed 3 weeks ago by gk

Keywords: tbb-needs added

comment:13 Changed 3 weeks ago by arma

Hint: in maint-0.3.5's src/feature/nodelist/networkstatus.c's networkstatus_read_cached_consensus_impl(), we call

  char *result = read_file_to_str(filename, RFTS_IGNORE_MISSING, NULL);

Note that we don't pass it the RFTS_BIN flag, which means tor_open_cloexec()'s open() ultimately uses O_TEXT rather than O_BINARY for its flags.

But 0.4.0's networkstatus_map_cached_consensus_impl()'s tor_mmap_file() doesn't have the same Windows notion of O_TEXT.

And on Linux, open() doesn't care if it's binary or text because they're the same thing, so Linux (and really everything other than Windows) continued to work when we shifted from text to binary in 0.4.0.

Of course, for extra fun notice that we seem to have a Windows-specific mmap implementation: see the #elif _WIN32 version of tor_mmap_file().

comment:14 Changed 3 weeks ago by arma

Or maybe I am looking at this bug the wrong way around: how did it come to have \r's in it on disk? I assume it's because we wrote it with Windows-style O_TEXT which added the \r's. So if we want it to not have \r's in it when we read it, we need to write it in O_BINARY mode too? And then there will still be the edge case where we try to load a legacy file which has \r's in it, and we should teach ourselves to handle that case better (e.g. discard it and fetch another right then)?

comment:15 Changed 3 weeks ago by nickm

So we need to do two things here:

  • always write a consensus in binary mode
  • on windows, when we encounter a consensus with CRLF in it, strip the CRs.

comment:16 Changed 3 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:17 Changed 3 weeks ago by nickm

Status: acceptedneeds_review

Proposed fix in bug28614_040, PR at https://github.com/torproject/tor/pull/671

comment:18 Changed 3 weeks ago by nickm

Priority: MediumHigh

comment:19 Changed 3 weeks ago by watt

- When reading a consensus fiel from disk, detect whether it
FILE

Could we avoid that and just ask users to clear the old file if it's wrong?

comment:20 Changed 3 weeks ago by ahf

Reviewer: ahf

comment:21 Changed 2 weeks ago by nickm

Could we avoid that and just ask users to clear the old file if it's wrong?

We could, but that would be inconvenience to the users, and it's best not to inconvenience users for a problem that we can fix.

Not everybody is comfortable navigating inside folders and deleting stuff.

comment:22 in reply to:  21 ; Changed 2 weeks ago by watt

Replying to nickm:

Could we avoid that and just ask users to clear the old file if it's wrong?

We could, but that would be inconvenience to the users, and it's best not to inconvenience users for a problem that we can fix.

Not everybody is comfortable navigating inside folders and deleting stuff.

No user intervention then. But adding more code to core tor just for transition to a new file format is not the best option. Will you remove it when tor is upgraded?

comment:23 Changed 2 weeks ago by gk

Cc: cypherpunks2 added

Closed #29302 as a duplicate

comment:24 in reply to:  22 ; Changed 2 weeks ago by nickm

Replying to watt:

Replying to nickm:

Could we avoid that and just ask users to clear the old file if it's wrong?

We could, but that would be inconvenience to the users, and it's best not to inconvenience users for a problem that we can fix.

Not everybody is comfortable navigating inside folders and deleting stuff.

No user intervention then. But adding more code to core tor just for transition to a new file format is not the best option. Will you remove it when tor is upgraded?

Probably; would you like to open a ticket for that? This code will be relevant until all earlier versions of Tor are obsolete, since people have any time between now and then to update. Since 0.3.5.x is supported till 2022, we shouldn't remove it before then.

comment:25 Changed 10 days ago by ahf

Status: needs_reviewmerge_ready

Tested this on Windows. Looks like it makes the 'ISO time "...\r" was unparseable' go away.

Code looks good.

comment:26 in reply to:  24 Changed 10 days ago by watt

Replying to nickm:

Replying to watt:

Replying to nickm:

Could we avoid that and just ask users to clear the old file if it's wrong?

We could, but that would be inconvenience to the users, and it's best not to inconvenience users for a problem that we can fix.

Not everybody is comfortable navigating inside folders and deleting stuff.

No user intervention then. But adding more code to core tor just for transition to a new file format is not the best option. Will you remove it when tor is upgraded?

Probably; would you like to open a ticket for that? This code will be relevant until all earlier versions of Tor are obsolete, since people have any time between now and then to update. Since 0.3.5.x is supported till 2022, we shouldn't remove it before then.

Usually such tickets are buried in the Tor: unspecified milestone. And it's almost impossible that somebody will remind us about it in 2022 :(
The main drawback is that tor should do additional parsing again and again at runtime instead of one time deletion during update.

comment:27 Changed 4 days ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged.

Note: See TracTickets for help on using tickets.