[tor-bugs] #1376 [Tor Client]: router_rebuild_store() uses a needless tmp file

[Tor Bug Tracker & Wiki]

#1376: router_rebuild_store() uses a needless tmp file
------------------------+---------------------------------------------------
 Reporter:  nickm       |          Owner:  pranavrc        
     Type:  defect      |         Status:  assigned        
 Priority:  trivial     |      Milestone:  Tor: unspecified
Component:  Tor Client  |        Version:  Tor: 0.2.2.35   
 Keywords:  easy        |         Parent:                  
   Points:              |   Actualpoints:                  
------------------------+---------------------------------------------------
Changes (by pranavrc):

  * owner:  => pranavrc
  * status:  new => assigned


Comment:

 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 [ticket:1376 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.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/1376#comment:4>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online