Opened 4 months ago

Last modified 2 months ago

#29930 new defect

Warning: can't unlink unverified-consensus on Windows

Reported by: teor Owned by: asn
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.4.0.3-alpha
Severity: Normal Keywords: 042-proposed nice-to-fix technical-debt
Cc: Vort Actual Points: 2
Parent ID: Points: 1
Reviewer: nickm Sponsor:

Description

In #28614, 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.

Child Tickets

Attachments (2)

run1.txt (3.6 KB) - added by Vort 4 months ago.
run2.txt (1.8 KB) - added by Vort 4 months ago.

Download all attachments as: .zip

Change History (13)

Changed 4 months ago by Vort

Attachment: run1.txt added

Changed 4 months ago by Vort

Attachment: run2.txt added

comment:1 Changed 4 months ago by Vort

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

comment:2 Changed 3 months ago by nickm

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.

comment:3 Changed 3 months ago by nickm

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

comment:4 Changed 3 months ago by ahf

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

comment:5 Changed 3 months ago by ahf

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?

comment:6 Changed 3 months ago by ahf

Actual Points: 2
Status: assignedneeds_review

comment:7 Changed 3 months ago by nickm

Keywords: asn-merge added
Reviewer: nickm
Status: needs_reviewmerge_ready

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.

comment:8 Changed 3 months ago by asn

Owner: changed from ahf to asn
Status: merge_readyaccepted

Merged to 040 and later.

comment:9 Changed 3 months ago by asn

Status: acceptednew

comment:10 Changed 3 months ago by asn

Keywords: asn-merge removed

comment:11 Changed 2 months ago by nickm

Keywords: 042-proposed nice-to-fix technical-debt added; 040-must Windows? removed
Milestone: Tor: 0.4.0.x-finalTor: unspecified
Note: See TracTickets for help on using tickets.