Bug#752872: libapr1: file locking is broken, leading to file corruption in e.g. libapache2-mod-auth-cas session files

2014-10-10 Thread Joost van Baal-Ilić 
Hi,

Stefan: Thanks for your time investigating this bug.

Op Sat, Aug 16, 2014 at 09:55:11PM +0200 schreef Stefan Fritsch:
 On Friday 27 June 2014 11:37:18, Joost van Baal-Ilić wrote:
  While libapr1 defaults to fcntl() locking it also supports flock(),
  which does not have the problems outlined above. A patch is
  attached which makes libapr1 use flock() even if fcntl() locking is
  available.
 
 flock does not support locking on NFS (at least according to its man 
 page), while fcntl does. I am undecided which is worse.
 
 Since there does not seem to be a good solution, I am downgrading the 
 severity of this bug.

The default behavior for Linux NFS clients is to convert flock() calls into
fcntl(). See nfs(5), in particular the local_lock option (the default for
that option is ‘none’). To summarize:

 - fcntl() locking is broken for everyone (even if you're using a non-threaded
   mpm you will still run into problems if you open the locked file twice);

 - flock() is broken only for people using NFS (rare) AND a non-Linux kernel
   (very rare, in Debian at least).

Therefore the proposed solution is good, imho.  Leaving severity to your
judgement, nevertheless.

Bye,

Joost



signature.asc
Description: Digital signature

Bug#752872: libapr1: file locking is broken, leading to file corruption in e.g. libapache2-mod-auth-cas session files

2014-08-16 Thread Stefan Fritsch 
severity 752872 important
found 752872 1.4.6-3
thanks

On Friday 27 June 2014 11:37:18, Joost van Baal-Ilić wrote:
 While libapr1 defaults to fcntl() locking it also supports flock(),
 which does not have the problems outlined above. A patch is
 attached which makes libapr1 use flock() even if fcntl() locking is
 available.

flock does not support locking on NFS (at least according to its man 
page), while fcntl does. I am undecided which is worse.

Since there does not seem to be a good solution, I am downgrading the 
severity of this bug.


--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org

Bug#752872: libapr1: file locking is broken, leading to file corruption in e.g. libapache2-mod-auth-cas session files

2014-06-27 Thread Joost van Baal-Ilić 
Package: libapr1
Version: 1.4.6-3+deb7u1
Severity: grave
Tags: patch, upstream

Hi,

libapr1 uses fcntl(F_SETLKW) for locking files, but this is not compatible
with multithreaded programs. fcntl(F_SETLKW) has the strange quirk that if
an open and locked file is opened and then closed a second time in the same
process, the lock is lost. This is something that may happen frequently in
multithreaded programs, such as the apache2 mpm worker.

fd1 = open(foo, O_RDWR|O_CREAT);
fcntl(fd1, F_SETLKW, ...);
/* file is now locked */
fd2 = open(foo, O_RDONLY);
/* file is still locked */
close(fd2);
/* file is no longer locked! */
...

Since file locking in libapr1 is broken^Wviolates the principle of least
surprise, dataloss can very likely happen.

I haven't checked the POSIX specs to see if this is expected behavior but I
was able to reproduce it on both Linux and FreeBSD. A patch is attached
that extends the libapr1 test suite to detect this situation.

While libapr1 defaults to fcntl() locking it also supports flock(), which
does not have the problems outlined above. A patch is attached which makes
libapr1 use flock() even if fcntl() locking is available.

We found this bug when investigating error messages from
libapache2-mod-auth-cas that its session files were getting corrupted:

 [error] [client 127.0.0.1] MOD_AUTH_CAS: Error parsing XML content for 
'01234567890abcdef01234567890abcd' (Internal error), referer: 
https://www.example.com/

Switching to the flock() mechanism solved these problems. In other words,
this bug is causing problems in real life, and is not just theoretical.

This bug was found, reported to me and patched by Wessel Dankers.

Thanks, Bye,

Joost van Baal-Ilić

-- 
Joost van Baal-Ilić   http://abramowitz.uvt.nl/
 Tilburg University
The Netherlands
diff -ur apr-1.4.6,orig/file_io/unix/flock.c apr-1.4.6,fixed/file_io/unix/flock.c
--- apr-1.4.6,orig/file_io/unix/flock.c	2006-08-03 12:55:31.0 +0200
+++ apr-1.4.6,fixed/file_io/unix/flock.c	2014-06-27 10:28:48.721611923 +0200
@@ -27,7 +27,7 @@
 {
 int rc;
 
-#if defined(HAVE_FCNTL_H)
+#if defined(HAVE_FCNTL_H)  0
 {
 struct flock l = { 0 };
 int fc;
diff -ur apr-1.4.6,orig/test/testflock.c apr-1.4.6,test/test/testflock.c
--- apr-1.4.6,orig/test/testflock.c	2010-03-07 16:06:47.0 +0100
+++ apr-1.4.6,test/test/testflock.c	2014-06-27 10:18:59.786062499 +0200
@@ -60,6 +60,7 @@
 static void test_withlock(abts_case *tc, void *data)
 {
 apr_file_t *file;
+apr_file_t *file2;
 apr_status_t rv;
 int code;
 
@@ -71,6 +72,12 @@
 rv = apr_file_lock(file, APR_FLOCK_EXCLUSIVE);
 APR_ASSERT_SUCCESS(tc, Could not lock the file., rv);
 ABTS_PTR_NOTNULL(tc, file);
+
+/* open and close the file another time, to see if that messes with things */
+rv = apr_file_open(file2, TESTFILE, APR_FOPEN_WRITE, APR_OS_DEFAULT, p);
+APR_ASSERT_SUCCESS(tc, Could not open file., rv);
+ABTS_PTR_NOTNULL(tc, file2);
+(void) apr_file_close(file2);
 
 code = launch_reader(tc);
 ABTS_INT_EQUAL(tc, FAILED_READ, code);


signature.asc
Description: Digital signature