Warning: can't unlink unverified-consensus on Windows

In #28614 (moved), Vort says:

BTW, clean start of 0.4.0.3-alpha brings another problem: Mar 28 08:26:00.000 [warn] Failed to unlink C:\Users\Vort\AppData\Roaming\tor\unverified-microdesc-consensus: Permission denied

Does this issue happen with an empty data directory? Or does it only happen when unverified-microdesc-consensus exists?

I'm assigning this ticket to ahf, because he has a working Windows box with Tor.

changed milestone to %Tor: unspecified

added 042-proposed actualpoints::2 component::core tor/tor milestone::Tor: unspecified nice-to-fix owner::asn points::1 priority::medium reviewer::nickm severity::normal status::new technical-debt type::defect version::tor 0.4.0.3-alpha labels

Trac:
Username: Vort

run1.txt

Trac:
Username: Vort

Trac:
Username: Vort

run2.txt

Trac:
Username: Vort

I see this message only with clean start. Here are the logs for two successive runs: [run1.txt​], [run2.txt​].

Trac:
Username: Vort

Annoyingly, we have this message in three different places in networkstatus.c, all in networkstatus_set_current_consensus(). We also give this error from src/lib/fs/files.c in finish_writing_to_file(). As a first step it might be good to figure out where this is happening from.

(ahf, if this takes more than a couple of hours, it's probably okay to remove the 040-must from the ticket.)

I reproduced it yesterday, going to try to figure out what is wrong later today.

Tried to debug this on Linux to see what could cause the file to remain opened at the time of the call to unlink(). On Linux the following system calls are made around the problematic time of execution:

 15 rename("/home/user/torrc-test//cached-certs.tmp", "/home/user/torrc-test//cached-certs") = 0
 14 openat(AT_FDCWD, "/home/user/torrc-test//unverified-microdesc-consensus", O_RDONLY|O_CLOEXEC) = 9
 13 fstat(9, {st_mode=S_IFREG|0600, st_size=2124535, ...}) = 0
 12 mmap(NULL, 2124535, PROT_READ, MAP_PRIVATE, 9, 0) = 0x77db35a74000
 11 close(9)                                = 0
    [ ... brk calls ... ]
  0 unlink("/home/user/torrc-test//unverified-microdesc-consensus") = 0

On Windows the tor_mmap_file() function will keep the file open until we call tor_munmap_file(). This is a problem in reload_consensus_from_file() since we will do the following:

1. mmap() the unverified-microdesc-consensus file.
2. Call networkstatus_set_current_consensus() with the content of the memory mapped file
3. networkstatus_set_current_consensus() will call unlink() on unverified-microdesc-consensus, which is still open because of (1) and will thus fail.
4. We will munmap() the file again when networkstatus_set_current_consensus() returns to reload_consensus_from_file().

I'm not sure what a good solution would be here. We need to either get rid of the mmap() call or lift the call to unlink() out of the block of code where the file that is to be unlinked is still memory mapped.

One option is to avoid using mmap(), but use read_file_to_str(). I think though that we moved to mmap() in this code to be more gentle with memory usage for iOS. Maybe we can use read_file_to_str() for Windows only, or?

Ready for review: https://github.com/torproject/tor/pull/978

Trac:
Actualpoints: N/A to 2
Status: assigned to needs_review

lgtm. Let's merge this to 0.4.0 and later, but then make this ticket "new" again and work on a better fix in 0.4.1 or later.

Trac:
Reviewer: N/A to nickm
Status: needs_review to merge_ready
Keywords: N/A deleted, asn-merge added

Merged to 040 and later.

Trac:
Owner: ahf to asn
Status: merge_ready to accepted

Trac:
Status: accepted to new

Trac:
Keywords: asn-merge deleted, N/A added

Trac:
Keywords: Windows?, 040-must deleted, 042-proposed nice-to-fix technical-debt added
Milestone: Tor: 0.4.0.x-final to Tor: unspecified

changed time estimate to 8h

added 16h of time spent

moved to tpo/core/tor#29930