[PATCH] Severe database corruption when running without standard file descriptors on unix

[Tobias Ringström <to...@ringstrom.mine.nu>]

A user approached us on IRC today with a severely broken repository. The 
problem was that a backup script was running "svnadmin dump" with it's 
standard input and standard error file descriptors closed. When BDB 
opened the nodes db, the file handle was 2, so when svnadmin printed 
it's status output to stderr, the output ended up in the nodes db file, 
which of course caused severe database corruption.

It is arguable whether this is a bug in svnadmin or not, but it's pretty 
easy to fix, and given the severity of the corruption, I think we should 
fix it and also backport it to 1.0.x and 1.1.x. The obvious solution is 
to check if file descriptors 0, 1 and 2 are closed, and make them point 
to /dev/null if they are. Attached to this email is a patch that 
implements this solution for all our command line tools, but there is a 
problem with the patch in that it's using #ifndef WIN32. The correct 
thing to do is of course to test for features instead of testing what 
platform it is not.

The patch uses a few unix specific include files, lstat(), open() and 
"/dev/null". What is the correct way to do handle this with autoconf? 
Should one check for all these one by one, or all of them? Any help is 
appreciated!

/Tobias

[[[
Prevent severe database corruption and other horrors on unix systems
if e.g. svnadmin is run with it's standard input, standard output or
standard error file descriptors closed.

* subversion/libsvn_subr/cmdline.c
  (svn_cmdline_init): Open '/dev/null' for standard input, output and
  error if they are closed.
]]]

Re: [PATCH] Severe database corruption when running without standard file descriptors on unix

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

On Fri, 2004-09-03 at 11:35, Tobias Ringström wrote:
> problem with the patch in that it's using #ifndef WIN32.

I don't really consider this a problem.  Feature testing is the right
thing when dealing with non-portable Unix features (because otherwise
you get into "AIX version 5 has this, but version 4 doesn't, and every
few years a new version of Unix comes along and you have to update the
tests"), but isn't so much the right thing when dealing with portable
Unix features which don't apply to Windows.

It can (and has been) argued that "#ifndef WIN32" is a dumb test for
Unix, but we use it all over the code base.  If and when we settle on a
better preprocessor test for it, we can do a global replacement, but
that's a larger issue than any particular bit of Unix-specific code.

The patch looks fine to me.  I don't think it's really an app's
responsibility to cope when invoked incorrectly like this, but it's
cheap and it obviously would have helped at least one person avoid
shooting himself in the foot.


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