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

[Joost van Baal-Ilić <joostvb+debian-bugs-20140627-9@uvt.nl>]

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.000000000 +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.000000000 +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);

Attachment: signature.asc
Description: Digital signature