Opened 3 years ago

Last modified 22 months ago

#19661 needs_revision enhancement

tor refuses to use /dev/null as a config file

Reported by: weasel Owned by: cypherpunks
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.8.5-rc
Severity: Normal Keywords: lorax, integration
Cc: Actual Points:
Parent ID: Points: .1
Reviewer: nickm Sponsor:

Description

sometimes it is useful to make sure tor does not load any values from a config file, and -f /dev/null is the default way to give it an empty one on linux.

Tor now refuses to use that as a file.

Child Tickets

Attachments (3)

0001-Support-S_IFCHR-in-file_status.patch (735 bytes) - added by cypherpunks 3 years ago.
0001-Support-S_IFCHR-in-file_status.2.patch (1.1 KB) - added by cypherpunks 2 years ago.
Revised patch to add changes file
fixup.patch (804 bytes) - added by cypherpunks 2 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 3 years ago by nickm

Keywords: easy lorax integration added
Milestone: Tor: 0.3.0.x-final
Points: .1

To fix this, switch the function that loads the torrc from read_file_to_str to use read_file_to_str_until_eof instead. A patch would be welcome. This could be a good intro task.

Changed 3 years ago by cypherpunks

comment:2 Changed 3 years ago by cypherpunks

It looks like this was actually failing because file_status does not currently support character special devices. The quickest fix was to just add support for this.

comment:3 Changed 3 years ago by cypherpunks

Status: newneeds_review

comment:4 Changed 3 years ago by nickm

Keywords: review-group-10 added

Add 0.3.0 needs_review items (and ones that haven't been in needs_revision for very long) to review-group-10.

comment:5 Changed 3 years ago by asn

Status: needs_reviewneeds_revision

The fix is correct here. Thanks cypherpunks!!!

However, it's missing a changes file. cypherpunks would you like to provide one? See here for info: https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n56

And also ideally provide a proper git branch, instead of a patches file?

Apart from that all is good here.

Last edited 3 years ago by asn (previous) (diff)

comment:6 Changed 3 years ago by asn

Hmmm, thought about this some more.

The patch here does not whitelist /dev/null specifically. Instead, it makes file_status() accept any character device and return it as "a non-empty regular file, or a FIFO on unix-like systems". We use file_status() in various places in the codebase, like when opening/saving the torrc, or state files, or crypto keys.

I wonder if there are any operations that we can't do with a character device file that are required for the features above (like seek, or unlink, or whatever).

I did some tests by setting my state file and my consensus file to be a character device, and everything worked fine because Tor just replaced them. I'm just wondering if there is some crazy Unix thing that might bite us here. But I guess this can only occur if our users specify a character device file themselves, so that's OK.

comment:7 Changed 3 years ago by teor

I've reviewed this patch, and it seems simple enough, and trivially correct.
And it does solve the problem in general for almost all files read by tor.

comment:8 Changed 3 years ago by nickm

Owner: set to cypherpunks
Status: needs_revisionassigned

comment:9 Changed 3 years ago by nickm

Status: assignedneeds_revision

comment:10 Changed 3 years ago by nickm

Keywords: review-group-10 removed

Changed 2 years ago by cypherpunks

Revised patch to add changes file

comment:11 Changed 2 years ago by cypherpunks

Status: needs_revisionneeds_review

comment:12 Changed 2 years ago by cypherpunks

The changes file is not in the expected format. This can be verified with make check-changes.

comment:13 Changed 2 years ago by cypherpunks

The related code was introduced in commit 92acbe12bc9512100b9282d7e9d61fe86b5a60bb which is Tor version 0.0.2pre13.

Changed 2 years ago by cypherpunks

Attachment: fixup.patch added

comment:14 Changed 2 years ago by cypherpunks

Attached a fixup patch which changes the header to the expected format, adds a category, reflows the text, adds the ticket number in the expected format and adds the Tor version.

comment:15 Changed 2 years ago by nickm

Keywords: review-group-14 added

comment:16 Changed 2 years ago by nickm

Reviewer: nickm

comment:17 Changed 2 years ago by nickm

I think it would be much better to stop using file_status() whenever we don't absolutely have to do so. That's for a couple of reasons:

  • The st_size field is not reliable on S_IFCHR devices, so anything that distinguishes between FN_EMPTY and FN_FILE will get the wrong answer.
  • Generally, when we want to see whether we can read a file, it's better to just try to read the file and see what happens. To do otherwise invites race conditions. The only time when it makes sense to have a separate "does this file exist" check is when we *aren't* going to open the file.

comment:18 Changed 2 years ago by nickm

Status: needs_reviewneeds_revision

comment:19 Changed 2 years ago by nickm

Keywords: review-group-15 added; review-group-14 removed

comment:20 Changed 2 years ago by nickm

Type: defectenhancement

batch modify: I think these are "enhancement", though I could be wrong about some.

comment:21 Changed 2 years ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final

comment:22 Changed 2 years ago by arma

The poor cypherpunks who has been trying to help here for 8 months -- and the ticket still has the 'easy' tag? :)

comment:23 Changed 2 years ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

0.3.1 feature freeze today: these don't seem like they will be all sorted out in time. Let's try to make progress in 0.3.2.

comment:24 Changed 2 years ago by nickm

Keywords: review-group-15 removed

comment:25 Changed 22 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: unspecified

Mark some needs_revision tickets as unspecified. If/when the revisions happen, they can go back into a live milestone.

comment:26 Changed 22 months ago by teor

Keywords: easy removed
Note: See TracTickets for help on using tickets.