Re: svn commit: r9085 - in trunk/subversion: include libsvn_subr libsvn_wc

[Greg Hudson <gh...@MIT.EDU>]

On Tue, 2004-03-16 at 17:59, Ben Reser wrote:
> > I think we have to keep the old implementation under the old name, and
> > introduce a new function name with the new behaviour.  By all means
> > write more explicit documentation disparaging the old interface ;-)
> 
> This was originally my opinion.  But everyone told me I was wrong off
> list.  If people feel differently due to the issue not being a security
> issue now then I'll be happy to make a different function.

To clarify my opinion on this matter:

Right now, svn_io_set_file_read_write() effectively does a chmod a+w.

I think it would be reasonable to make it do a chmod u+w and treat that
as a bugfix rather than an API change.  It may be a good idea to do this
in svn 1.0.x, for the marginal security value.  (Most of the files we
set to read-write are then immediately deleted, so it doesn't matter if
anyone writes to them first; we believe the only exception is the format
file, and there isn't a lot of utility in seizing the short window of
writability to write data into the format file.)

I don't think it would be reasonable to make it into a no-op.

If we want to enable shared working directories (in 1.1), perhaps the
best option is to introduce a function private to libsvn_wc, which
invokes svn_io_set_file_read_write only on Windows.  (Our ABI rules
allow us to do this in svn 1.0.x, but it's a new feature, not a bugfix,
so we shouldn't do so.)


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r9085 - in trunk/subversion: include libsvn_subr libsvn_wc

[Philip Martin <ph...@codematters.co.uk>]

Greg Hudson <gh...@MIT.EDU> writes:

> (Alternatively, we could treat it as a bug that svn_io_remove_file()
> doesn't work on read-only files under Windows, and just make it do so,
> but that might affect our performance if we start issuing set-attr calls
> before every file deletion on Windows.)

We have already done that with a #ifdef WIN32 in svn_io_remove_file.
There is an open issue about changing this, but we have never decided
how else the platform specific code should be handled.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r9085 - in trunk/subversion: include libsvn_subr libsvn_wc

[Ben Reser <be...@reser.org>]

On Tue, Mar 16, 2004 at 06:38:51PM -0500, Greg Hudson wrote:
> On Tue, 2004-03-16 at 18:31, Ben Reser wrote:
> > > If we want to enable shared working directories (in 1.1), perhaps the
> > > best option is to introduce a function private to libsvn_wc, which
> > > invokes svn_io_set_file_read_write only on Windows.
> 
> > Actually there is code in other sections that would need this same
> > behavior.  I think we should just add a function in 1.1 that gives us a
> > place to put things that need to be done before a file is removed.
> 
> I feel like "prep file for removal" is not a good candidate for a real
> API.  I'd be a bit happier seeing an API for "remove a file which we
> expect is read-only," which could perhaps be called
> svn_io_remove_read_only_file().

Actually there is good reason to put this in a separate API and my
quoting of the name was because of that reason.

There are 3 cases where we need to make a read-only file writable on
Windows before we can carry out an operation:

1) removal
2) renaming
3) replacing/overwriting with a new file.

We could make new functions for remove (which already does this now),
copy, rename, and a open that force a +w.  Or we could make one new
function that does things we'd want to do before one of these operations
on a read-only file.  I just haven't come up with a good name for it
yet.

> (Alternatively, we could treat it as a bug that svn_io_remove_file()
> doesn't work on read-only files under Windows, and just make it do so,
> but that might affect our performance if we start issuing set-attr calls
> before every file deletion on Windows.)

Actually svn_io_remove_file() already handles removing read-only files
on Windows.

-- 
Ben Reser <be...@reser.org>
http://ben.reser.org

"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r9085 - in trunk/subversion: include libsvn_subr libsvn_wc

[Greg Hudson <gh...@MIT.EDU>]

On Tue, 2004-03-16 at 18:31, Ben Reser wrote:
> > If we want to enable shared working directories (in 1.1), perhaps the
> > best option is to introduce a function private to libsvn_wc, which
> > invokes svn_io_set_file_read_write only on Windows.

> Actually there is code in other sections that would need this same
> behavior.  I think we should just add a function in 1.1 that gives us a
> place to put things that need to be done before a file is removed.

I feel like "prep file for removal" is not a good candidate for a real
API.  I'd be a bit happier seeing an API for "remove a file which we
expect is read-only," which could perhaps be called
svn_io_remove_read_only_file().

(Alternatively, we could treat it as a bug that svn_io_remove_file()
doesn't work on read-only files under Windows, and just make it do so,
but that might affect our performance if we start issuing set-attr calls
before every file deletion on Windows.)


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r9085 - in trunk/subversion: include libsvn_subr libsvn_wc

[Ben Reser <be...@reser.org>]

On Tue, Mar 16, 2004 at 06:07:13PM -0500, Greg Hudson wrote:
> Right now, svn_io_set_file_read_write() effectively does a chmod a+w.
> 
> I think it would be reasonable to make it do a chmod u+w and treat that
> as a bugfix rather than an API change.  It may be a good idea to do this
> in svn 1.0.x, for the marginal security value.  (Most of the files we
> set to read-write are then immediately deleted, so it doesn't matter if
> anyone writes to them first; we believe the only exception is the format
> file, and there isn't a lot of utility in seizing the short window of
> writability to write data into the format file.)

Correct...  I think I'd rather just leave well enough alone in 1.0.x.
Introduce a new function 1.1.x to "prep for removal" that does nothing
on Unix.  But calls read_write on Windows.  Put a big fat warning on
read_write about what it really does on Unix and to be careful with it.

> If we want to enable shared working directories (in 1.1), perhaps the
> best option is to introduce a function private to libsvn_wc, which
> invokes svn_io_set_file_read_write only on Windows.

Actually there is code in other sections that would need this same
behavior.  I think we should just add a function in 1.1 that gives us a
place to put things that need to be done before a file is removed.
Right now the only OS that needs this is Windows.  But if we find other
OSes that need something like this we'll be set.

> (Our ABI rules allow us to do this in svn 1.0.x, but it's a new
> feature, not a bugfix, so we shouldn't do so.)

Unless you consider shared working copies not working to be a bug. :)
(Not saying I do but I'm sure someone will say that they do).

-- 
Ben Reser <be...@reser.org>
http://ben.reser.org

"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org