Opened 10 years ago

Closed 6 years ago

Last modified 6 years ago

#1376 closed defect (fixed)

router_rebuild_store() uses a needless tmp file

Reported by: nickm Owned by: pranavrc
Priority: Very Low Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.2.35
Severity: Keywords: easy tor-client
Cc: grarpamp@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In router_rebuild_store(), we use write_chunks_to_file() to build cached_descriptors.tmp, and if that is successful, we rename cached_descriptors.tmp to cached_descriptors.

But write_chunks_to_file() *already* uses a tmpfile to make sure that it doesn't trash the file it's writing to, so we wind up with the following situation:

router_rebuild_store() calls

write_chunks_to_file(), which does:

open("cached_descriptors.tmp.tmp")
write
rename("cached_descriptors.tmp.tmp","cached_descriptors.tmp")

munmap("cached_descriptors")
rename("cached_descriptors.tmp", "cached_descriptors")
mmap("cached_descriptors")

One imaginable fix would be to have router_rebuild_store() tell write_chunks_to_file to write into cached_descriptors directly, so that it would write into c-d.tmp then rename it to c-d. That won't work so easily, though: on Windows, you're not allowed to replace a mapped file while it's still mapped.

A better fix would be to add a flag to write_chunks_to_file() so that it takes a flag that decides whether to use a tmpfile or not.

Found by grarpamp on or-talk: http://archives.seul.org/or/talk/Mar-2010/msg00173.html

This bug isn't actually hurting anything by doing an extra rename(), so it isn't urgent to fix.

Child Tickets

Change History (18)

comment:1 Changed 8 years ago by grarpamp

Cc: grarpamp@… added
Milestone: Tor: unspecifiedTor: 0.2.3.x-final
Version: Tor: unspecifiedTor: 0.2.2.35

Noticed <basename>.tmp.tmp yet being created.
Suggest also try moving all temporary file management
to the secure mkstemp(3) or platform equiv.
Updating version info to 35.

comment:2 Changed 8 years ago by tomfitzhenry

Cc: trac-tor@… added

comment:3 Changed 7 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: unspecified

comment:4 in reply to:  description Changed 7 years ago by pranavrc

Owner: set to pranavrc
Status: newassigned

New contributor here, willing to work on this bug. I'll assign it to myself if that's okay. I have a couple of queries though. write_chunks_to_file_impl() calls start_writing_to_file(), which in turn appends '.tmp' to the filename, here:

tor_asprintf(&new_file->tempname, "%s.tmp", fname);

so I assume that's where the filename becomes 'cached_descriptors.tmp.tmp'. I'm afraid I don't understand what is meant by 'so that it takes a flag to decide whether to use a tmpfile or not.', because write_chunks_to_file_impl() seems to be opening only one file stream to write the data chunks to.

Awaiting a response, cheers :)

Replying to nickm:

In router_rebuild_store(), we use write_chunks_to_file() to build cached_descriptors.tmp, and if that is successful, we rename cached_descriptors.tmp to cached_descriptors.

But write_chunks_to_file() *already* uses a tmpfile to make sure that it doesn't trash the file it's writing to, so we wind up with the following situation:

router_rebuild_store() calls

write_chunks_to_file(), which does:

open("cached_descriptors.tmp.tmp")
write
rename("cached_descriptors.tmp.tmp","cached_descriptors.tmp")

munmap("cached_descriptors")
rename("cached_descriptors.tmp", "cached_descriptors")
mmap("cached_descriptors")

One imaginable fix would be to have router_rebuild_store() tell write_chunks_to_file to write into cached_descriptors directly, so that it would write into c-d.tmp then rename it to c-d. That won't work so easily, though: on Windows, you're not allowed to replace a mapped file while it's still mapped.

A better fix would be to add a flag to write_chunks_to_file() so that it takes a flag that decides whether to use a tmpfile or not.

Found by grarpamp on or-talk: http://archives.seul.org/or/talk/Mar-2010/msg00173.html

This bug isn't actually hurting anything by doing an extra rename(), so it isn't urgent to fix.

comment:5 Changed 7 years ago by nickm

The flag would be to select between the current behavior ("open X.tmp, write to it, and later rename it to X") and a different behavior ("open X and write to it directly.")

comment:6 Changed 7 years ago by nickm

Keywords: tor-client added

comment:7 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:8 Changed 6 years ago by Ry

Added additional parameter and updated callers.
Please review: https://github.com/Ryman/tor/tree/bug1376

comment:9 Changed 6 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.5.x-final
Status: assignedneeds_review

comment:10 Changed 6 years ago by tomfitzhenry

Cc: trac-tor@… removed

comment:11 Changed 6 years ago by nickm

Hm. This looks plausible. I wonder how we could write a test for this? I don't see an obvious way to ensure that no tempfile is created.

comment:12 Changed 6 years ago by Ry

If we assume we're always going to have .tmp as our added extension we could write our own temp file with a random string and after a call we can ensure the temp file wasn't modified. Could then verify that the tempfile does get modified if we don't set the flag. It's quite flakey though but better than nothing I guess?

comment:13 Changed 6 years ago by nickm

Sure, that sounds okay to me.

comment:15 Changed 6 years ago by nickm

Thanks! I've tightened it up a little in 7ef9ecf6b38041e38dd079c2d18b5c8813d1959a and merged it.

comment:16 Changed 6 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:17 Changed 6 years ago by arma

Is this a fair description of the result of the ticket?

diff --git a/changes/bug1376 b/changes/bug1376
index bee42a3..531a68e 100644
--- a/changes/bug1376
+++ b/changes/bug1376
@@ -1,4 +1,3 @@
-  o Minor bugfixes:
-
-    - Added additional argument to write_chunks_to_file to optionally skip
-      using a temp file to do non-atomic writes. Implements ticket #1376.
+  o Code simplification and refactoring: 
+    - Previously we used two temporary files when writing descriptors to
+      disk; now we only use one. Implements ticket 1376.

comment:18 Changed 6 years ago by nickm

Yes. Or if you want to be more amusing:

    - Previously we used a temporary file when writing descriptors to disk,
      and another temporary file to write the temporary file; now we only use
      one. Implements ticket 1376.
Note: See TracTickets for help on using tickets.