You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by bo...@apache.org on 2007/04/18 04:14:17 UTC
svn commit: r529830 - in /apr/apr/branches/1.2.x: CHANGES
file_io/unix/readwrite.c test/testfile.c
Author: bojan
Date: Tue Apr 17 19:14:16 2007
New Revision: 529830
URL: http://svn.apache.org/viewvc?view=rev&rev=529830
Log:
Backport r524823, r524822, r524821, r524355 from the trunk
Fix deadlock in apr_file_gets() for a file opened with both the
APR_BUFFERED and APR_XTHREAD flags
Modified:
apr/apr/branches/1.2.x/CHANGES
apr/apr/branches/1.2.x/file_io/unix/readwrite.c
apr/apr/branches/1.2.x/test/testfile.c
Modified: apr/apr/branches/1.2.x/CHANGES
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.2.x/CHANGES?view=diff&rev=529830&r1=529829&r2=529830
==============================================================================
--- apr/apr/branches/1.2.x/CHANGES (original)
+++ apr/apr/branches/1.2.x/CHANGES Tue Apr 17 19:14:16 2007
@@ -1,6 +1,7 @@
Changes for APR 1.2.9
-
+ *) Fix deadlock in apr_file_gets() for a file opened with both the
+ APR_BUFFERED and APR_XTHREAD flags. [Bojan Smojver]
Changes for APR 1.2.8
Modified: apr/apr/branches/1.2.x/file_io/unix/readwrite.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.2.x/file_io/unix/readwrite.c?view=diff&rev=529830&r1=529829&r2=529830
==============================================================================
--- apr/apr/branches/1.2.x/file_io/unix/readwrite.c (original)
+++ apr/apr/branches/1.2.x/file_io/unix/readwrite.c Tue Apr 17 19:14:16 2007
@@ -25,6 +25,62 @@
#define USE_WAIT_FOR_IO
#endif
+static apr_status_t file_read_buffered(apr_file_t *thefile, void *buf,
+ apr_size_t *nbytes)
+{
+ apr_ssize_t rv;
+ char *pos = (char *)buf;
+ apr_uint64_t blocksize;
+ apr_uint64_t size = *nbytes;
+
+ if (thefile->direction == 1) {
+ rv = apr_file_flush(thefile);
+ if (rv) {
+ return rv;
+ }
+ thefile->bufpos = 0;
+ thefile->direction = 0;
+ thefile->dataRead = 0;
+ }
+
+ rv = 0;
+ if (thefile->ungetchar != -1) {
+ *pos = (char)thefile->ungetchar;
+ ++pos;
+ --size;
+ thefile->ungetchar = -1;
+ }
+ while (rv == 0 && size > 0) {
+ if (thefile->bufpos >= thefile->dataRead) {
+ int bytesread = read(thefile->filedes, thefile->buffer, APR_FILE_BUFSIZE);
+ if (bytesread == 0) {
+ thefile->eof_hit = TRUE;
+ rv = APR_EOF;
+ break;
+ }
+ else if (bytesread == -1) {
+ rv = errno;
+ break;
+ }
+ thefile->dataRead = bytesread;
+ thefile->filePtr += thefile->dataRead;
+ thefile->bufpos = 0;
+ }
+
+ blocksize = size > thefile->dataRead - thefile->bufpos ? thefile->dataRead - thefile->bufpos : size;
+ memcpy(pos, thefile->buffer + thefile->bufpos, blocksize);
+ thefile->bufpos += blocksize;
+ pos += blocksize;
+ size -= blocksize;
+ }
+
+ *nbytes = pos - (char *)buf;
+ if (*nbytes) {
+ rv = 0;
+ }
+ return rv;
+}
+
APR_DECLARE(apr_status_t) apr_file_read(apr_file_t *thefile, void *buf, apr_size_t *nbytes)
{
apr_ssize_t rv;
@@ -36,66 +92,14 @@
}
if (thefile->buffered) {
- char *pos = (char *)buf;
- apr_uint64_t blocksize;
- apr_uint64_t size = *nbytes;
-
#if APR_HAS_THREADS
if (thefile->thlock) {
apr_thread_mutex_lock(thefile->thlock);
}
#endif
- if (thefile->direction == 1) {
- rv = apr_file_flush(thefile);
- if (rv) {
-#if APR_HAS_THREADS
- if (thefile->thlock) {
- apr_thread_mutex_unlock(thefile->thlock);
- }
-#endif
- return rv;
- }
- thefile->bufpos = 0;
- thefile->direction = 0;
- thefile->dataRead = 0;
- }
+ rv = file_read_buffered(thefile, buf, nbytes);
- rv = 0;
- if (thefile->ungetchar != -1) {
- *pos = (char)thefile->ungetchar;
- ++pos;
- --size;
- thefile->ungetchar = -1;
- }
- while (rv == 0 && size > 0) {
- if (thefile->bufpos >= thefile->dataRead) {
- int bytesread = read(thefile->filedes, thefile->buffer, APR_FILE_BUFSIZE);
- if (bytesread == 0) {
- thefile->eof_hit = TRUE;
- rv = APR_EOF;
- break;
- }
- else if (bytesread == -1) {
- rv = errno;
- break;
- }
- thefile->dataRead = bytesread;
- thefile->filePtr += thefile->dataRead;
- thefile->bufpos = 0;
- }
-
- blocksize = size > thefile->dataRead - thefile->bufpos ? thefile->dataRead - thefile->bufpos : size;
- memcpy(pos, thefile->buffer + thefile->bufpos, blocksize);
- thefile->bufpos += blocksize;
- pos += blocksize;
- size -= blocksize;
- }
-
- *nbytes = pos - (char *)buf;
- if (*nbytes) {
- rv = 0;
- }
#if APR_HAS_THREADS
if (thefile->thlock) {
apr_thread_mutex_unlock(thefile->thlock);
@@ -364,7 +368,7 @@
}
else {
nbytes = 1;
- rv = apr_file_read(thefile, str, &nbytes);
+ rv = file_read_buffered(thefile, str, &nbytes);
if (rv != APR_SUCCESS) {
break;
}
Modified: apr/apr/branches/1.2.x/test/testfile.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.2.x/test/testfile.c?view=diff&rev=529830&r1=529829&r2=529830
==============================================================================
--- apr/apr/branches/1.2.x/test/testfile.c (original)
+++ apr/apr/branches/1.2.x/test/testfile.c Tue Apr 17 19:14:16 2007
@@ -386,6 +386,29 @@
apr_file_close(f);
}
+static void test_gets_buffered(abts_case *tc, void *data)
+{
+ apr_file_t *f = NULL;
+ apr_status_t rv;
+ char *str = apr_palloc(p, 256);
+
+ /* This will deadlock gets before the r524355 fix. */
+ rv = apr_file_open(&f, FILENAME, APR_READ|APR_BUFFERED|APR_XTHREAD, 0, p);
+ ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+
+ rv = apr_file_gets(str, 256, f);
+ /* Only one line in the test file, so APR will encounter EOF on the first
+ * call to gets, but we should get APR_SUCCESS on this call and
+ * APR_EOF on the next.
+ */
+ ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ ABTS_STR_EQUAL(tc, TESTSTR, str);
+ rv = apr_file_gets(str, 256, f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ ABTS_STR_EQUAL(tc, "", str);
+ apr_file_close(f);
+}
+
static void test_bigread(abts_case *tc, void *data)
{
apr_file_t *f = NULL;
@@ -853,6 +876,7 @@
abts_run_test(suite, test_getc, NULL);
abts_run_test(suite, test_ungetc, NULL);
abts_run_test(suite, test_gets, NULL);
+ abts_run_test(suite, test_gets_buffered, NULL);
abts_run_test(suite, test_puts, NULL);
abts_run_test(suite, test_writev, NULL);
abts_run_test(suite, test_writev_full, NULL);
Re: svn commit: r529830 - in /apr/apr/branches/1.2.x: CHANGES
file_io/unix/readwrite.c test/testfile.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Wed, 2007-04-18 at 14:08 +1000, Bojan Smojver wrote:
> OK. I'm sure Joe will have better ideas (as usual :-), but I'm sure it
> won't hurt if I have a look as well.
And most certainly he did. I think it's time for folks to look at the
Bugzilla entry and maybe add a comment or two in relation to "should
this be fixed, or should it be left the way it is". As Joe pointed out,
the fix for this is easy, but whether the issue should be fixed or not
is a different matter.
http://issues.apache.org/bugzilla/show_bug.cgi?id=41119
--
Bojan
Re: svn commit: r529830 - in /apr/apr/branches/1.2.x: CHANGES
file_io/unix/readwrite.c test/testfile.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Tue, 2007-04-17 at 21:58 -0500, William A. Rowe, Jr. wrote:
> I plan to at some point 'nearly close' 0.9 by creating another stable
> release. If you wanted to backport I'd be most appreciative.
Looked at 0.9.x code and it seems that apr_file_gets() doesn't use any
locks, but relies on apr_file_read() completely (i.e. it doesn't even
have a separate buffered code path). So, I'm guessing this deadlock
isn't affecting this version.
I can still commit Joe's apr_file_gets() buffered test, if you like (I
ported that and it seems to pass). Yes?
> If you or Joe would look at
>
> http://issues.apache.org/bugzilla/show_bug.cgi?id=41119
>
> there is definately some evil interaction between buffering and fork()
> for exec().
OK. I'm sure Joe will have better ideas (as usual :-), but I'm sure it
won't hurt if I have a look as well.
--
Bojan
Re: svn commit: r529830 - in /apr/apr/branches/1.2.x: CHANGES
file_io/unix/readwrite.c test/testfile.c
Posted by Bojan Smojver <bo...@rexursive.com>.
On Tue, 2007-04-17 at 21:58 -0500, William A. Rowe, Jr. wrote:
> http://issues.apache.org/bugzilla/show_bug.cgi?id=41119
A fairly corny resolution suggestion posted in the bug report itself.
--
Bojan
Re: svn commit: r529830 - in /apr/apr/branches/1.2.x: CHANGES file_io/unix/readwrite.c
test/testfile.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Humble requests (2)
I plan to at some point 'nearly close' 0.9 by creating another stable
release. If you wanted to backport I'd be most appreciative.
If you or Joe would look at
http://issues.apache.org/bugzilla/show_bug.cgi?id=41119
there is definately some evil interaction between buffering and fork()
for exec().
Re: svn commit: r529830 - in /apr/apr/branches/1.2.x: CHANGES file_io/unix/readwrite.c
test/testfile.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Humble requests (2)
I plan to at some point 'nearly close' 0.9 by creating another stable
release. If you wanted to backport I'd be most appreciative.
If you or Joe would look at
http://issues.apache.org/bugzilla/show_bug.cgi?id=41119
there is definately some evil interaction between buffering and fork()
for exec().