Hi everyone, Currently, the Windows implementation of APR_FOPEN_APPEND flag performs a seek to the file's end when opening it.
Apparently, this is a leftover from the very first version of handling file appends (https://svn.apache.org/r59449) that performed a single seek to the file's end when opening it and did not support proper atomic appends with multiple process or threads writing to the same file. Since then, such atomic appends have been implemented, but the seek was not removed. It can cause unexpected behavior when reading from a file opened with APR_FOPEN_APPEND, assuming no writes happened to the file. In this case, as there have been no writes, the file offset should not be repositioned and reading should start from the beginning of the file. However, due to the unwanted seek during open, the actual reading would instead start from the file's end and cause an unexpected EOF. The attached patch fixes the issue and adds a test for it. The log message is included in the beginning of the patch file. Thanks, Evgeny Kotkov
Win32: Don't seek to the end when opening files with APR_FOPEN_APPEND. Apparently, this is a leftover from the very first version of handling file appends (https://svn.apache.org/r59449) that performed a single seek to the file's end when opening it and did not support proper atomic appends with multiple process or threads writing to the same file. Since then, such atomic appends have been implemented, but the seek was not removed. It can cause unexpected behavior when reading from a file opened with APR_FOPEN_APPEND, assuming no writes happened to the file. In this case, as there have been no writes, the file offset should not be repositioned and reading should start from the beginning of the file. However, due to the unwanted seek during open, the actual reading would instead start from the file's end and cause an unexpected EOF. * file_io/win32/open.c (apr_file_open): Don't seek to the file's end when the file is opened with APR_FOPEN_APPEND. * test/testfile.c (test_append_read): New test. (testfile): Run the new test. Patch by: Evgeny Kotkov <evgeny.kotkov {at} visualsvn.com> Index: file_io/win32/open.c =================================================================== --- file_io/win32/open.c (revision 1806645) +++ file_io/win32/open.c (working copy) @@ -445,7 +445,6 @@ APR_DECLARE(apr_status_t) apr_file_open(apr_file_t if (flag & APR_FOPEN_APPEND) { (*new)->append = 1; - SetFilePointer((*new)->filehand, 0, NULL, FILE_END); } if (flag & APR_FOPEN_BUFFERED) { (*new)->buffered = 1; Index: test/testfile.c =================================================================== --- test/testfile.c (revision 1806645) +++ test/testfile.c (working copy) @@ -1793,6 +1793,56 @@ static void test_append_locked(abts_case *tc, void apr_file_remove(fname, p); } +static void test_append_read(abts_case *tc, void *data) +{ + apr_status_t rv; + apr_file_t *f; + const char *fname = "data/testappend_read.dat"; + apr_off_t offset; + char buf[64]; + + apr_file_remove(fname, p); + + /* Create file with contents. */ + rv = apr_file_open(&f, fname, APR_FOPEN_WRITE | APR_FOPEN_CREATE, + APR_FPROT_OS_DEFAULT, p); + APR_ASSERT_SUCCESS(tc, "create file", rv); + + rv = apr_file_puts("abc", f); + APR_ASSERT_SUCCESS(tc, "write to file", rv); + apr_file_close(f); + + /* Re-open it with APR_FOPEN_APPEND. */ + rv = apr_file_open(&f, fname, + APR_FOPEN_READ | APR_FOPEN_WRITE | APR_FOPEN_APPEND, + APR_FPROT_OS_DEFAULT, p); + APR_ASSERT_SUCCESS(tc, "open file", rv); + + /* Test the initial file offset. Even though we used APR_FOPEN_APPEND, + * the offset should be kept in the beginning of the file until the + * first append. (Previously, the Windows implementation performed + * an erroneous seek to the file's end right after opening it.) + */ + offset = 0; + rv = apr_file_seek(f, APR_CUR, &offset); + APR_ASSERT_SUCCESS(tc, "get file offset", rv); + ABTS_INT_EQUAL(tc, 0, (int)offset); + + /* Test reading from the file. */ + rv = apr_file_gets(buf, sizeof(buf), f); + APR_ASSERT_SUCCESS(tc, "read from file", rv); + ABTS_STR_EQUAL(tc, "abc", buf); + + /* Test the file offset after reading. */ + offset = 0; + rv = apr_file_seek(f, APR_CUR, &offset); + APR_ASSERT_SUCCESS(tc, "get file offset", rv); + ABTS_INT_EQUAL(tc, 3, (int)offset); + + apr_file_close(f); + apr_file_remove(fname, p); +} + abts_suite *testfile(abts_suite *suite) { suite = ADD_SUITE(suite) @@ -1848,6 +1898,7 @@ abts_suite *testfile(abts_suite *suite) abts_run_test(suite, test_write_buffered_spanning_over_bufsize, NULL); abts_run_test(suite, test_atomic_append, NULL); abts_run_test(suite, test_append_locked, NULL); + abts_run_test(suite, test_append_read, NULL); return suite; }
