`svn co` creates files with wrong permissions

[Arfrever Frehtes Taifersar Arahesis-2]

`svn co` (and `svn up`) creates files with wrong permissions. This problem
doesn't occur with Subversion 1.6.x@HEAD, so it's a regression in trunk.
I use Subversion trunk@40332.

$ LC_ALL=C ~/subversion-wrong_permissions.sh
+ umask 0022
+ umask
0022
+ rm -fr repo wc1 wc2
+ svnadmin create repo
++ pwd
+ svn co file:///tmp/repo wc1
Checked out revision 0.
+ pushd wc1
+ touch file
+ svn add file
A         file
+ svn ci -m ''
Adding         file
Transmitting file data .
Committed revision 1.
+ popd
++ pwd
+ svn co file:///tmp/repo wc2
A    wc2/file
Checked out revision 1.
+ ls -l --time-style=+%Y-%m-%d-%H-%M-%S wc1 wc2
wc1:
total 0
-rw-r--r-- 1 Arfrever Arfrever 0 2009-11-01-02-29-31 file

wc2:
total 0
-rw------- 1 Arfrever Arfrever 0 2009-11-01-02-29-32 file

--
Arfrever Frehtes Taifersar Arahesis

[Arfrever Frehtes Taifersar Arahesis-2]

2009-11-01 02:34:06 Arfrever Frehtes Taifersar Arahesis napisał(a):
> `svn co` (and `svn up`) creates files with wrong permissions. This problem
> doesn't occur with Subversion 1.6.x@HEAD, so it's a regression in trunk.
> I use Subversion trunk@40332.

This regression has been introduced in r40264.

--
Arfrever Frehtes Taifersar Arahesis

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413373

[Stefan Sperling-7]

On Sun, Nov 01, 2009 at 03:23:29AM +0100, Arfrever Frehtes Taifersar Arahesis wrote:
> 2009-11-01 02:34:06 Arfrever Frehtes Taifersar Arahesis napisał(a):
> > `svn co` (and `svn up`) creates files with wrong permissions. This problem
> > doesn't occur with Subversion 1.6.x@HEAD, so it's a regression in trunk.
> > I use Subversion trunk@40332.
>
> This regression has been introduced in r40264.

This happens because we now use mkstemp() to create temporary files
in many places, and mkstemp() locks down permissions to 0600. The temp
file is then copied to the working copy and permissions aren't adjusted.
So the (very old) bug you ran into is that instead of adjusting permissions,
svn is using the permissions of the tempfile unaltered, even though those
permissions might not make sense in the WC.
Note that on win32 all files inherit their perms from their parent
folder and we have no way to override this yet. So this problem only
happens on UNIX.

I ran into a similar problem before, see r40275. Specifically, see the change
made to libsvn_wc/copy.c:copy_file_administratively in r40275. It shows an
example of how the problem can be fixed by doing a single svn_io_copy_perms()
call after installing the temp file in the working copy. If there is no
source file to copy perms from (you can even pass NULL for 'src'), the
destination will automatically get default permissions, which should honour
the umask.

Stefan

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413398

[Stefan Sperling-7]

On Sun, Nov 01, 2009 at 09:59:25AM +0100, Stefan Sperling wrote:

> On Sun, Nov 01, 2009 at 03:23:29AM +0100, Arfrever Frehtes Taifersar Arahesis wrote:
> > 2009-11-01 02:34:06 Arfrever Frehtes Taifersar Arahesis napisał(a):
> > > `svn co` (and `svn up`) creates files with wrong permissions. This problem
> > > doesn't occur with Subversion 1.6.x@HEAD, so it's a regression in trunk.
> > > I use Subversion trunk@40332.
> >
> > This regression has been introduced in r40264.
>
> This happens because we now use mkstemp() to create temporary files
> in many places, and mkstemp() locks down permissions to 0600. The temp
> file is then copied to the working copy and permissions aren't adjusted.
> So the (very old) bug you ran into is that instead of adjusting permissions,
> svn is using the permissions of the tempfile unaltered, even though those
> permissions might not make sense in the WC.
> Note that on win32 all files inherit their perms from their parent
> folder and we have no way to override this yet. So this problem only
> happens on UNIX.
>
> I ran into a similar problem before, see r40275. Specifically, see the change
> made to libsvn_wc/copy.c:copy_file_administratively in r40275. It shows an
> example of how the problem can be fixed by doing a single svn_io_copy_perms()
> call after installing the temp file in the working copy. If there is no
> source file to copy perms from (you can even pass NULL for 'src'), the
> destination will automatically get default permissions, which should honour
> the umask.

I have fixed the problem shown by your example script in r40333.
Thanks for catching this.

We may want to look for more places where temporary files end up being
installed in the working copy and make sure to always set permissions
properly. Anyone up for a bite-sized task like this? Scanning the code
or use of temporary files should give anticipating developers lots of
opportunity to learn.

Stefan

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413400

[Daniel Näslund]

> We may want to look for more places where temporary files end up being
> installed in the working copy and make sure to always set permissions
> properly. Anyone up for a bite-sized task like this? Scanning the code
> or use of temporary files should give anticipating developers lots of
> opportunity to learn.

I took the bait! Started grepping and thought that I might just as well
keep what I've found here as on my hard drive.

On Sun, Nov 01, 2009 at 11:13:44AM +0100, Stefan Sperling wrote:

> >
> > This happens because we now use mkstemp() to create temporary files
> > in many places, and mkstemp() locks down permissions to 0600. The temp
> > file is then copied to the working copy and permissions aren't adjusted.
> > So the (very old) bug you ran into is that instead of adjusting permissions,
> > svn is using the permissions of the tempfile unaltered, even though those
> > permissions might not make sense in the WC.
> > Note that on win32 all files inherit their perms from their parent
> > folder and we have no way to override this yet. So this problem only
> > happens on UNIX.

I found that the mkstemp system call is passed up this way:

[[[
svn_subst_copy_and_translate3()
svn_io_write_unique(), svn_stream_open_unique()
svn_io_open_unique_file3()
svn_io_file_mktemp()
apr_file_mktemp()
mkstemp()
]]]

I assume that I need not to concern myself with the permissions of files
stored in the adm area? They are subject to fast change and should not
be accessed directly by the user anyway, right?

I'm not really sure how the loggy mechanism works. I have assumed that
it's like a journal of what has been done to be used for undoing things.
Like how ext3 and a database works. Correct?

So the places in libsvn_wc that uses svn_io_open_unique_file3:

[[[
* adm_files.c
  (svn_wc_create_tmp_file2): OK. Depreceated. In the adm area.

* merge.c
  (detranslate_wc_file): Uses mkstemp perms.
  (maybe_update_target_eols): Uses mkstemp perms.
  (preserve_pre_merge_files) OK. Copies permissions.

* translate.c
  (svn_wc__internal_translated_file): Uses mkstemp perms.
    May be depreceated says comment.

* diff.c
  (get_empty_file): OK. A cached file in the edit_baton to be reused for
    different purposes.
]]]

svn_stream_open_unique is used here:

[[[
* adm_crawler.c
  (restore_file): Uses mkstemp perms.

* props.c
  (loggy_write_properties): OK(?). I haven't really grasped the loggy
    functionality yet. It creates a file for holding properties. I
    assume that it's in the adm area.

  (open_reject_tmp_stream): OK(?). Assume this is the the stream for a
    tmp file in adm area.

* update_editor.c
  (get_empty_tmp_file): OK(?). The tmp area of .svn.
  (add_file_with_history): Uses mkstemp perms.
  (svn_wc_add_repos_file4): OK.

* merge.c
  (eval_conflict_func_result): Uses mkstemp perms. Saves the resulting
    three way diff after the conflict resolution callback has done it's
    job.

* conflicts.c
  (resolve_conflict_on_node): Uses mkstemp perms. Write diff3 to the
    newly created stream.

* diff.c
  (apply_textdelta): OK. Created in admin area.
]]]

svn_subst_copy_and_translate3() is used here:

[[[
* merge.c
  (detranslate_wc_file): Uses mkstemp perms.
  (maybe_update_target_eols): Uses mkstemp perms.

* translate.c
  (svn_wc__internal_translated_file): Uses mkstemp perms.

* log.c
  (file_xfer_under_path): OK.

* workqueue.c
  (copy_and_translate): Not sure. The workqueue is something very
    mysterious to me.
]]]

I've found nine places where the umask is not honoured and the 0600 of
mkstemp is used instead.

/Daniel

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413484

[Stefan Sperling-7]

On Sun, Nov 01, 2009 at 07:53:00PM +0100, Daniel Näslund wrote:
> > We may want to look for more places where temporary files end up being
> > installed in the working copy and make sure to always set permissions
> > properly. Anyone up for a bite-sized task like this? Scanning the code
> > or use of temporary files should give anticipating developers lots of
> > opportunity to learn.
>
> I took the bait! Started grepping and thought that I might just as well
> keep what I've found here as on my hard drive.

Thanks!

> I found that the mkstemp system call is passed up this way:
>
> [[[
> svn_subst_copy_and_translate3()
> svn_io_write_unique(), svn_stream_open_unique()
> svn_io_open_unique_file3()
> svn_io_file_mktemp()
> apr_file_mktemp()
> mkstemp()
> ]]]
>

That's a nice list.

I have a patch in progress now which will solve this problem within
svn_io_open_unique_file3(), to repair the breakage caused. We can still
fix all the callers copying/renaming tempfiles into WORKING, however,
and then svn_io_open_unique_file3() won't have to bother with permissions.

Stefan

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2413517