Hi everyone,
This patch fixes two issues with apr_file_trunc() for buffered files:
- The Win32 implementation incorrectly flushes the buffered writes _after_
truncating a file. Such files will have unexpected data written after
the position at which they should've been truncated. (Under Unix,
this issue has been fixed in https://svn.apache.org/r1044440)
- Both Win32 and Unix implementations incorrectly keep the data read into
a buffer after the file is truncated. Thus, reading from a file after
apr_file_trunc() can return invalid data from the previous file offset.
Log message:
[[[
Fix two issues with apr_file_trunc() for buffered files:
- The Win32 implementation incorrectly flushes the buffered writes _after_
truncating a file. Such files will have unexpected data written after
the position at which they should've been truncated. (Under Unix,
this issue has been fixed in https://svn.apache.org/r1044440)
- Both Win32 and Unix implementations incorrectly keep the data read into
a buffer after the file is truncated. Thus, reading from a file after
apr_file_trunc() can return invalid data from the previous file offset.
* file_io/win32/seek.c
(apr_file_trunc): Flush the write buffer or discard the read buffer
before truncating. Propely update the internal file offset (filePtr)
and the eof_hit marker.
* file_io/unix/seek.c
(apr_file_trunc): Discard the read buffer before truncating.
* test/testfile.c
(test_file_trunc): Extend the checks. Factor out part of this test...
(test_file_trunc_buffered_write): ...into this new test.
(test_file_trunc_buffered_write2, test_file_trunc_buffered_read): New tests.
(testfile): Run the new tests.
Patch by: Evgeny Kotkov <evgeny.kotkov {at} visualsvn.com>
]]]
Regards,
Evgeny Kotkov
Index: file_io/unix/seek.c
===================================================================
--- file_io/unix/seek.c (revision 1784526)
+++ file_io/unix/seek.c (working copy)
@@ -117,6 +117,13 @@ apr_status_t apr_file_trunc(apr_file_t *fp, apr_of
/* Reset buffer positions for write mode */
fp->bufpos = fp->direction = fp->dataRead = 0;
}
+ else if (fp->direction == 0) {
+ /* Discard the read buffer, as we are about to reposition
+ * ourselves to the end of file.
+ */
+ fp->bufpos = 0;
+ fp->dataRead = 0;
+ }
file_unlock(fp);
if (rc) {
return rc;
Index: file_io/win32/seek.c
===================================================================
--- file_io/win32/seek.c (revision 1784526)
+++ file_io/win32/seek.c (working copy)
@@ -161,17 +161,43 @@ APR_DECLARE(apr_status_t) apr_file_trunc(apr_file_
LONG offhi = (LONG)(offset >> 32);
DWORD rc;
+ if (thefile->buffered) {
+ if (thefile->direction == 1) {
+ /* Figure out what needs to be flushed. Don't flush the part
+ * of the write buffer that will get truncated anyway.
+ */
+ if (offset < thefile->filePtr) {
+ thefile->bufpos = 0;
+ }
+ else if (offset < thefile->filePtr + (apr_off_t)thefile->bufpos) {
+ thefile->bufpos = offset - thefile->filePtr;
+ }
+
+ if (thefile->bufpos != 0) {
+ rv = apr_file_flush(thefile);
+ if (rv != APR_SUCCESS)
+ return rv;
+ }
+ }
+ else if (thefile->direction == 0) {
+ /* Discard the read buffer, as we are about to reposition
+ * ourselves to the end of file.
+ */
+ thefile->bufpos = 0;
+ thefile->dataRead = 0;
+ }
+ }
+
rc = SetFilePointer(thefile->filehand, offlo, &offhi, FILE_BEGIN);
if (rc == 0xFFFFFFFF)
if ((rv = apr_get_os_error()) != APR_SUCCESS)
return rv;
+ thefile->filePtr = offset;
+ /* Don't report EOF until the next read. */
+ thefile->eof_hit = 0;
if (!SetEndOfFile(thefile->filehand))
return apr_get_os_error();
- if (thefile->buffered) {
- return setptr(thefile, offset);
- }
-
return APR_SUCCESS;
}
Index: test/testfile.c
===================================================================
--- test/testfile.c (revision 1784526)
+++ test/testfile.c (working copy)
@@ -822,10 +822,10 @@ static void test_file_trunc(abts_case *tc, void *d
const char *s;
apr_size_t nbytes;
apr_finfo_t finfo;
+ char c;
apr_file_remove(fname, p);
- /* Test unbuffered */
rv = apr_file_open(&f, fname,
APR_FOPEN_CREATE | APR_FOPEN_READ |
APR_FOPEN_WRITE,
@@ -839,15 +839,40 @@ static void test_file_trunc(abts_case *tc, void *d
ABTS_SIZE_EQUAL(tc, strlen(s), nbytes);
rv = apr_file_trunc(f, 4);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ /* Test apr_file_info_get(). */
+ rv = apr_file_info_get(&finfo, APR_FINFO_SIZE, f);
+ ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ ABTS_INT_EQUAL(tc, 4, (int)finfo.size);
+ /* EOF is not reported until the next read. */
+ rv = apr_file_eof(f);
+ ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ rv = apr_file_getc(&c, f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ rv = apr_file_eof(f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+
rv = apr_file_close(f);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
rv = apr_stat(&finfo, fname, APR_FINFO_SIZE, p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
- ABTS_ASSERT(tc, "File size mismatch, expected 4", finfo.size == 4);
+ ABTS_INT_EQUAL(tc, 4, (int)finfo.size);
rv = apr_file_remove(fname, p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
- /* Test buffered */
+}
+
+static void test_file_trunc_buffered_write(abts_case *tc, void *data)
+{
+ apr_status_t rv;
+ apr_file_t *f;
+ const char *fname = "data/testtruncate_buffered_write.dat";
+ const char *s;
+ apr_size_t nbytes;
+ apr_finfo_t finfo;
+ char c;
+
+ apr_file_remove(fname, p);
+
rv = apr_file_open(&f, fname,
APR_FOPEN_CREATE | APR_FOPEN_READ |
APR_FOPEN_WRITE | APR_FOPEN_BUFFERED,
@@ -854,6 +879,7 @@ static void test_file_trunc(abts_case *tc, void *d
APR_FPROT_UREAD | APR_FPROT_UWRITE, p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ s = "some data";
nbytes = strlen(s);
rv = apr_file_write(f, s, &nbytes);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
@@ -860,16 +886,125 @@ static void test_file_trunc(abts_case *tc, void *d
ABTS_SIZE_EQUAL(tc, strlen(s), nbytes);
rv = apr_file_trunc(f, 4);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ /* Test apr_file_info_get(). */
+ rv = apr_file_info_get(&finfo, APR_FINFO_SIZE, f);
+ ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ ABTS_INT_EQUAL(tc, 4, (int)finfo.size);
+ /* EOF is not reported until the next read. */
+ rv = apr_file_eof(f);
+ ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ rv = apr_file_getc(&c, f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ rv = apr_file_eof(f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+
rv = apr_file_close(f);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
rv = apr_stat(&finfo, fname, APR_FINFO_SIZE, p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
- ABTS_ASSERT(tc, "File size mismatch, expected 4", finfo.size == 4);
+ ABTS_INT_EQUAL(tc, 4, (int)finfo.size);
rv = apr_file_remove(fname, p);
ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
}
+static void test_file_trunc_buffered_write2(abts_case *tc, void *data)
+{
+ apr_status_t rv;
+ apr_file_t *f;
+ const char *fname = "data/testtruncate_buffered_write2.dat";
+ apr_finfo_t finfo;
+ char c;
+
+ apr_file_remove(fname, p);
+
+ rv = apr_file_open(&f, fname,
+ APR_FOPEN_CREATE | APR_FOPEN_READ |
+ APR_FOPEN_WRITE | APR_FOPEN_BUFFERED,
+ APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "open test file", rv);
+
+ rv = apr_file_puts("abc", f);
+ APR_ASSERT_SUCCESS(tc, "write first string", rv);
+ rv = apr_file_flush(f);
+ APR_ASSERT_SUCCESS(tc, "flush", rv);
+ rv = apr_file_puts("def", f);
+ APR_ASSERT_SUCCESS(tc, "write second string", rv);
+ /* Truncate behind the write buffer. */
+ rv = apr_file_trunc(f, 2);
+ APR_ASSERT_SUCCESS(tc, "truncate the file", rv);
+ /* Test apr_file_info_get(). */
+ rv = apr_file_info_get(&finfo, APR_FINFO_SIZE, f);
+ APR_ASSERT_SUCCESS(tc, "get file info", rv);
+ ABTS_INT_EQUAL(tc, 2, (int)finfo.size);
+ /* EOF is not reported until the next read. */
+ rv = apr_file_eof(f);
+ ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ rv = apr_file_getc(&c, f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ rv = apr_file_eof(f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+
+ apr_file_close(f);
+
+ rv = apr_stat(&finfo, fname, APR_FINFO_SIZE, p);
+ APR_ASSERT_SUCCESS(tc, "stat file", rv);
+ ABTS_INT_EQUAL(tc, 2, (int)finfo.size);
+
+ apr_file_remove(fname, p);
+}
+
+static void test_file_trunc_buffered_read(abts_case *tc, void *data)
+{
+ apr_status_t rv;
+ apr_file_t *f;
+ const char *fname = "data/testtruncate_buffered_read.dat";
+ apr_finfo_t finfo;
+ char c;
+
+ apr_file_remove(fname, p);
+
+ rv = apr_file_open(&f, fname,
+ APR_FOPEN_CREATE | APR_FOPEN_READ |
+ APR_FOPEN_WRITE, APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "open test file", rv);
+
+ rv = apr_file_puts("abc", f);
+ APR_ASSERT_SUCCESS(tc, "write test data", rv);
+ apr_file_close(f);
+
+ rv = apr_file_open(&f, fname,
+ APR_FOPEN_READ | APR_FOPEN_WRITE |
+ APR_FOPEN_BUFFERED, APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "re-open test file", rv);
+
+ /* Read to fill in the buffer. */
+ rv = apr_file_getc(&c, f);
+ APR_ASSERT_SUCCESS(tc, "read char", rv);
+ /* Truncate the file. */
+ rv = apr_file_trunc(f, 1);
+ APR_ASSERT_SUCCESS(tc, "truncate the file", rv);
+ /* Test apr_file_info_get(). */
+ rv = apr_file_info_get(&finfo, APR_FINFO_SIZE, f);
+ APR_ASSERT_SUCCESS(tc, "get file info", rv);
+ ABTS_INT_EQUAL(tc, 1, (int)finfo.size);
+ /* EOF is not reported until the next read. */
+ rv = apr_file_eof(f);
+ ABTS_INT_EQUAL(tc, APR_SUCCESS, rv);
+ rv = apr_file_getc(&c, f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ rv = apr_file_eof(f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+
+ apr_file_close(f);
+
+ rv = apr_stat(&finfo, fname, APR_FINFO_SIZE, p);
+ APR_ASSERT_SUCCESS(tc, "stat file", rv);
+ ABTS_INT_EQUAL(tc, 1, (int)finfo.size);
+
+ apr_file_remove(fname, p);
+}
+
static void test_bigfprintf(abts_case *tc, void *data)
{
apr_file_t *f;
@@ -1134,6 +1269,9 @@ abts_suite *testfile(abts_suite *suite)
abts_run_test(suite, test_mod_neg, NULL);
abts_run_test(suite, test_truncate, NULL);
abts_run_test(suite, test_file_trunc, NULL);
+ abts_run_test(suite, test_file_trunc_buffered_write, NULL);
+ abts_run_test(suite, test_file_trunc_buffered_write2, NULL);
+ abts_run_test(suite, test_file_trunc_buffered_read, NULL);
abts_run_test(suite, test_bigfprintf, NULL);
abts_run_test(suite, test_fail_write_flush, NULL);
abts_run_test(suite, test_fail_read_flush, NULL);
