[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Re: Is mod_dav_svn safe for use in a threaded MPM?

From: Malcolm Rowe <malcolm-svn-dev_at_farside.org.uk>
Date: 2007-04-10 16:12:53 CEST

On Fri, Apr 06, 2007 at 04:20:24PM -0700, Eric Gillespie wrote:
> OK, i've implemented my proposal, except i have not removed the
> 'dav' directory. It's part of the public svn_repos_t structure.
> Can i just leave that field there (and continue to fill it in),
> but just stop creating the directory?
>

svn_repos_t is an opaque structure - we can change it however we want.

We could stop creating the 'dav' directory, but then you'd have a
situation where 'svnadmin create' couldn't create a repository that
worked with an older mod_dav_svn, I think. You could create the
directory only for --pre-1.x-compatible repostiories, though.
Is there no reason you can't just continue to use the existing 'dav'
directory?

> Stop using apr_dbm to manage the DAV activities database which maps

> Make the location of this directory configurable with the new

I think these should probably be two separate patches.

I've not reviewed this thoroughly, but I've got some obvious questions:

- Does mod_dav restrict activity names to a filename-safe subset?
  What's to stop someone performing a MKACTIVITY/MERGE against an activity
  called '../../../../etc/passwd', for example (or 'NUL' or 'AUX'...)?

- Are we happy to decide that the activities are tied to a specific
  mod_dav_svn version? i.e. we don't need to provide for compatibility
  between mod_dav_svn 1.5 and activities created with mod_dav_svn 1.4.

- I find it rather confusing that you're referring to an 'activities db'
  when it's not actually a database :-)

> Index: subversion/mod_dav_svn/activity.c
> ===================================================================
> +static const char *
> +_read_txn(const dav_svn_repos *repos, const char *pathname)
> {
> +
> + status = apr_stat(&finfo, pathname, APR_FINFO_SIZE, repos->pool);
> + _CHECK_STATUS;
> +
> + status = apr_file_open(&activity_file, pathname, APR_READ | APR_BUFFERED,
> + APR_OS_DEFAULT, repos->pool);

Opening the file in buffered mode when you're going to read everything
at once just wastes a 4k APR file buffer.

> +const char *
> +dav_svn__get_txn(const dav_svn_repos *repos, const char *activity_id)
> +{
> + const char *pathname;
> + pathname = svn_path_join(repos->activities_db, activity_id, repos->pool);

As I mentioned above, this seems unsafe to me given that the client
controls the activity id :-)

Regards,
Malcolm

  • application/pgp-signature attachment: stored
Received on Tue Apr 10 16:14:10 2007

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.