Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6044 closed defect (implemented)

Tor can't use a named pipe for a hidden service key

Reported by: katmagic Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs
Cc: meejah@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

If I try to use a named pipe for a hidden service key, Tor dies with

Jun 03 00:00:00.000 [notice] Tor 0.2.3.15-alpha-dev (git-ec7fd08ccfa878c3) opening log file.
Jun 03 00:00:00.000 [err] {FS} Can't read key from /tmp/tmp.SekAugJpK0/private_key"
Jun 03 00:00:00.000 [warn] {GENERAL} Error loading rendezvous service keys
Jun 03 00:00:00.000 [err] {BUG} set_options(): Bug: Acting on config options left us in a broken state. Dying.

Child Tickets

Change History (16)

comment:1 Changed 7 years ago by rransom

4806#comment:8 is relevant.

comment:2 Changed 7 years ago by meejah

Cc: meejah@… added
Status: newneeds_review

I have an implementation and test that will allow reading of private keys from FIFOs. As mentioned on IRC, I still have a question about file_status(): in my patch I'm just pretending that both regular files and FIFOs are FN_FILE but perhaps you want to treat them separately by adding to the file_status_t enum...?

There's also an un-squashed version of this, but I figured one commit is easier to review:

https://github.com/meejah/tor/commit/a90a93e0dce0cade00931b1e12c8d551f17eb072

I've only tried this on linux machines.

comment:3 Changed 7 years ago by meejah

The branch name in the above repo is 6044_read_whole_file_squashed (there's also one without _squashed on the end)

comment:4 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-final
Status: needs_reviewneeds_revision

Reviewing...

The memcpy seems unnecessary; you should be able to just read() into the string directly.

Reallocing the string every 256 bytes will give bad performance with some allocators; generally you want to reallocate on a schedule that gives you O(lg n) reallocs

Those last two won't actually matter in practice for the fifos you'd want to use this on; they're just QOI issues.

I'm a little worried about huge broken fifos running us out of RAM; maybe this should have an upper bound as an argument?

Make sure that it builds when you pass "--enable-gcc-warnings" to the compiler.

"char *string = 0;" should be "char *string = NULL;"

This function fails badly on a 0-byte fifo: it'll never allocate the string, but it'll call string[0] = '\0' anyway.

The test at the end of the loop is pointless; it can never be false. Instead you can just use while(1).

I think perhaps the function name wants to be different; it's not FIFO-specific, it's for anything where you want to read until you hit an EOF.

For making a file in the unit tests, you can use get_fname().

The unit test should probably be coded such that it tests the case that that goes through the loop just once, more than once, and not at all.

comment:5 Changed 7 years ago by nickm

oh, and I'm pretty sure that win32 doesn't have S_ISFIFO. So maybe that whole "if (S_ISFIFO..." block should be wrapped in #ifdef S_ISFIFO.

comment:6 Changed 7 years ago by meejah

Status: needs_revisionneeds_review

Okay, I think I covered all the changes above with a new branch: https://github.com/meejah/tor/tree/6044_read_whole_file_squashed_2

I haven't tried this on anything except linux still. I added some more tests and fixed all the above points (I think). The reading loop re-alloc's every kilobyte which is probably still overkill, but balance that with wasting memory (and the fact the main use-case is private_keys, which will now be read in one loop).

(process question: should I assign a ticket I'm working on like this to myself?)

comment:7 Changed 7 years ago by nickm

Looking better! I made some small tweaks to it in branch 6044_nm in my public repository. How does that look to you?

(process question: should I assign a ticket I'm working on like this to myself?)

Feel free if you like; I don't personally use the assigned-to field, but I know other people do.

comment:8 in reply to:  6 ; Changed 7 years ago by rransom

Replying to meejah:

I haven't tried this on anything except linux still. I added some more tests and fixed all the above points (I think). The reading loop re-alloc's every kilobyte which is probably still overkill, but balance that with wasting memory (and the fact the main use-case is private_keys, which will now be read in one loop).

naif wants to use this for every file in Tor's data directory.

comment:9 in reply to:  8 ; Changed 7 years ago by nickm

Replying to rransom:

Replying to meejah:

I haven't tried this on anything except linux still. I added some more tests and fixed all the above points (I think). The reading loop re-alloc's every kilobyte which is probably still overkill, but balance that with wasting memory (and the fact the main use-case is private_keys, which will now be read in one loop).

naif wants to use this for every file in Tor's data directory.

How does that make any sense? It would be madness to use this for cached-*.

comment:10 in reply to:  9 Changed 7 years ago by rransom

Replying to nickm:

Replying to rransom:

naif wants to use this for every file in Tor's data directory.

How does that make any sense? It would be madness to use this for cached-*.

It doesn't make any sense. (That said, he also wants to do that on Windows, where the FIFO trick isn't going to work.)

comment:11 Changed 7 years ago by rransom

Remember to add a changes/ file.

comment:12 Changed 7 years ago by nickm

Oh man; I wrote one but didn't commit it. It's 04c693832f2928ce8f01bf6ba now.

comment:13 Changed 7 years ago by meejah

The additions and changes in 6044_nm look great to me.

comment:14 Changed 7 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

make 6044_nm_squashed then merged it.

comment:15 Changed 7 years ago by nickm

Keywords: tor-hs added

comment:16 Changed 7 years ago by nickm

Component: Tor Hidden ServicesTor
Note: See TracTickets for help on using tickets.