You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by ko...@apache.org on 2023/02/07 10:44:12 UTC
svn commit: r1907495 - in /apr/apr/branches/1.7.x: ./ CHANGES file_io/win32/readwrite.c test/testfile.c
Author: kotkov
Date: Tue Feb 7 10:44:12 2023
New Revision: 1907495
URL: http://svn.apache.org/viewvc?rev=1907495&view=rev
Log:
On 1.7.x branch: Merge r1785072, r1788930 from trunk:
apr_file_gets: Optimize for buffered files on Windows.
Modified:
apr/apr/branches/1.7.x/ (props changed)
apr/apr/branches/1.7.x/CHANGES
apr/apr/branches/1.7.x/file_io/win32/readwrite.c
apr/apr/branches/1.7.x/test/testfile.c
Propchange: apr/apr/branches/1.7.x/
------------------------------------------------------------------------------
Merged /apr/apr/trunk:r1785072,1788930
Modified: apr/apr/branches/1.7.x/CHANGES
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/CHANGES?rev=1907495&r1=1907494&r2=1907495&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/CHANGES [utf-8] (original)
+++ apr/apr/branches/1.7.x/CHANGES [utf-8] Tue Feb 7 10:44:12 2023
@@ -12,6 +12,9 @@ Changes for APR 1.7.2
*) Correct a packaging issue in 1.7.1. The contents of the release were
correct, but the top level directory was misnamed.
+ *) apr_file_gets: Optimize for buffered files on Windows.
+ [Evgeny Kotkov <evgeny.kotkov visualsvn.com>]
+
Changes for APR 1.7.1
*) SECURITY: CVE-2022-24963 (cve.mitre.org)
Modified: apr/apr/branches/1.7.x/file_io/win32/readwrite.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/file_io/win32/readwrite.c?rev=1907495&r1=1907494&r2=1907495&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/file_io/win32/readwrite.c (original)
+++ apr/apr/branches/1.7.x/file_io/win32/readwrite.c Tue Feb 7 10:44:12 2023
@@ -140,6 +140,56 @@ static apr_status_t read_with_timeout(ap
return rv;
}
+static apr_status_t read_buffered(apr_file_t *thefile, void *buf, apr_size_t *len)
+{
+ apr_status_t rv;
+ char *pos = (char *)buf;
+ apr_size_t blocksize;
+ apr_size_t size = *len;
+
+ if (thefile->direction == 1) {
+ rv = apr_file_flush(thefile);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ thefile->bufpos = 0;
+ thefile->direction = 0;
+ thefile->dataRead = 0;
+ }
+
+ rv = 0;
+ while (rv == 0 && size > 0) {
+ if (thefile->bufpos >= thefile->dataRead) {
+ apr_size_t read;
+ rv = read_with_timeout(thefile, thefile->buffer,
+ thefile->bufsize, &read);
+ if (read == 0) {
+ if (rv == APR_EOF)
+ thefile->eof_hit = TRUE;
+ break;
+ }
+ else {
+ thefile->dataRead = read;
+ 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;
+ }
+
+ *len = pos - (char *)buf;
+ if (*len) {
+ rv = APR_SUCCESS;
+ }
+
+ return rv;
+}
+
APR_DECLARE(apr_status_t) apr_file_read(apr_file_t *thefile, void *buf, apr_size_t *len)
{
apr_status_t rv;
@@ -177,57 +227,10 @@ APR_DECLARE(apr_status_t) apr_file_read(
}
}
if (thefile->buffered) {
- char *pos = (char *)buf;
- apr_size_t blocksize;
- apr_size_t size = *len;
-
if (thefile->flags & APR_FOPEN_XTHREAD) {
apr_thread_mutex_lock(thefile->mutex);
}
-
- if (thefile->direction == 1) {
- rv = apr_file_flush(thefile);
- if (rv != APR_SUCCESS) {
- if (thefile->flags & APR_FOPEN_XTHREAD) {
- apr_thread_mutex_unlock(thefile->mutex);
- }
- return rv;
- }
- thefile->bufpos = 0;
- thefile->direction = 0;
- thefile->dataRead = 0;
- }
-
- rv = 0;
- while (rv == 0 && size > 0) {
- if (thefile->bufpos >= thefile->dataRead) {
- apr_size_t read;
- rv = read_with_timeout(thefile, thefile->buffer,
- thefile->bufsize, &read);
- if (read == 0) {
- if (rv == APR_EOF)
- thefile->eof_hit = TRUE;
- break;
- }
- else {
- thefile->dataRead = read;
- 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;
- }
-
- *len = pos - (char *)buf;
- if (*len) {
- rv = APR_SUCCESS;
- }
-
+ rv = read_buffered(thefile, buf, len);
if (thefile->flags & APR_FOPEN_XTHREAD) {
apr_thread_mutex_unlock(thefile->mutex);
}
@@ -455,30 +458,107 @@ APR_DECLARE(apr_status_t) apr_file_puts(
APR_DECLARE(apr_status_t) apr_file_gets(char *str, int len, apr_file_t *thefile)
{
- apr_size_t readlen;
apr_status_t rv = APR_SUCCESS;
- int i;
-
- for (i = 0; i < len-1; i++) {
- readlen = 1;
- rv = apr_file_read(thefile, str+i, &readlen);
+ apr_size_t nbytes;
+ const char *str_start = str;
+ char *final = str + len - 1;
- if (rv != APR_SUCCESS && rv != APR_EOF)
+ /* If the file is open for xthread support, allocate and
+ * initialize the overlapped and io completion event (hEvent).
+ * Threads should NOT share an apr_file_t or its hEvent.
+ */
+ if ((thefile->flags & APR_FOPEN_XTHREAD) && !thefile->pOverlapped) {
+ thefile->pOverlapped = (OVERLAPPED*) apr_pcalloc(thefile->pool,
+ sizeof(OVERLAPPED));
+ thefile->pOverlapped->hEvent = CreateEvent(NULL, FALSE, FALSE, NULL);
+ if (!thefile->pOverlapped->hEvent) {
+ rv = apr_get_os_error();
return rv;
+ }
+ }
- if (readlen == 0) {
- /* If we have bytes, defer APR_EOF to the next call */
- if (i > 0)
- rv = APR_SUCCESS;
- break;
+ /* Handle the ungetchar if there is one. */
+ if (thefile->ungetchar != -1 && str < final) {
+ *str = thefile->ungetchar;
+ thefile->ungetchar = -1;
+ if (*str == '\n') {
+ *(++str) = '\0';
+ return APR_SUCCESS;
}
-
- if (str[i] == '\n') {
- i++; /* don't clobber this char below */
- break;
+ ++str;
+ }
+
+ /* If we have an underlying buffer, we can be *much* more efficient
+ * and skip over the read_with_timeout() calls.
+ */
+ if (thefile->buffered) {
+ if (thefile->flags & APR_FOPEN_XTHREAD) {
+ apr_thread_mutex_lock(thefile->mutex);
}
+
+ if (thefile->direction == 1) {
+ rv = apr_file_flush(thefile);
+ if (rv) {
+ if (thefile->flags & APR_FOPEN_XTHREAD) {
+ apr_thread_mutex_unlock(thefile->mutex);
+ }
+ return rv;
+ }
+
+ thefile->direction = 0;
+ thefile->bufpos = 0;
+ thefile->dataRead = 0;
+ }
+
+ while (str < final) { /* leave room for trailing '\0' */
+ if (thefile->bufpos < thefile->dataRead) {
+ *str = thefile->buffer[thefile->bufpos++];
+ }
+ else {
+ nbytes = 1;
+ rv = read_buffered(thefile, str, &nbytes);
+ if (rv != APR_SUCCESS) {
+ break;
+ }
+ }
+ if (*str == '\n') {
+ ++str;
+ break;
+ }
+ ++str;
+ }
+ if (thefile->flags & APR_FOPEN_XTHREAD) {
+ apr_thread_mutex_unlock(thefile->mutex);
+ }
+ }
+ else {
+ while (str < final) { /* leave room for trailing '\0' */
+ nbytes = 1;
+ rv = read_with_timeout(thefile, str, nbytes, &nbytes);
+ if (rv == APR_EOF)
+ thefile->eof_hit = TRUE;
+
+ if (rv != APR_SUCCESS) {
+ break;
+ }
+ if (*str == '\n') {
+ ++str;
+ break;
+ }
+ ++str;
+ }
+ }
+
+ /* We must store a terminating '\0' if we've stored any chars. We can
+ * get away with storing it if we hit an error first.
+ */
+ *str = '\0';
+ if (str > str_start) {
+ /* We stored chars; don't report EOF or any other errors;
+ * the app will find out about that on the next call.
+ */
+ return APR_SUCCESS;
}
- str[i] = 0;
return rv;
}
Modified: apr/apr/branches/1.7.x/test/testfile.c
URL: http://svn.apache.org/viewvc/apr/apr/branches/1.7.x/test/testfile.c?rev=1907495&r1=1907494&r2=1907495&view=diff
==============================================================================
--- apr/apr/branches/1.7.x/test/testfile.c (original)
+++ apr/apr/branches/1.7.x/test/testfile.c Tue Feb 7 10:44:12 2023
@@ -430,6 +430,10 @@ static void test_gets(abts_case *tc, voi
rv = apr_file_gets(str, 256, f);
ABTS_INT_EQUAL(tc, APR_EOF, rv);
ABTS_STR_EQUAL(tc, "", str);
+ /* Calling gets after EOF should return EOF. */
+ rv = apr_file_gets(str, 256, f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ ABTS_STR_EQUAL(tc, "", str);
apr_file_close(f);
}
@@ -453,6 +457,214 @@ static void test_gets_buffered(abts_case
rv = apr_file_gets(str, 256, f);
ABTS_INT_EQUAL(tc, APR_EOF, rv);
ABTS_STR_EQUAL(tc, "", str);
+ /* Calling gets after EOF should return EOF. */
+ 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_gets_empty(abts_case *tc, void *data)
+{
+ apr_status_t rv;
+ apr_file_t *f;
+ const char *fname = "data/testgets_empty.txt";
+ char buf[256];
+
+ apr_file_remove(fname, p);
+
+ rv = apr_file_open(&f, fname, APR_FOPEN_CREATE | APR_FOPEN_WRITE,
+ APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "open test file", rv);
+ apr_file_close(f);
+
+ rv = apr_file_open(&f, fname, APR_FOPEN_READ, APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "re-open test file", rv);
+
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ ABTS_STR_EQUAL(tc, "", buf);
+ /* Calling gets after EOF should return EOF. */
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ ABTS_STR_EQUAL(tc, "", buf);
+ apr_file_close(f);
+}
+
+static void test_gets_multiline(abts_case *tc, void *data)
+{
+ apr_status_t rv;
+ apr_file_t *f;
+ const char *fname = "data/testgets_multiline.txt";
+ char buf[256];
+
+ apr_file_remove(fname, p);
+
+ rv = apr_file_open(&f, fname, APR_FOPEN_CREATE | APR_FOPEN_WRITE,
+ APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "open test file", rv);
+ rv = apr_file_puts("a\nb\n", f);
+ APR_ASSERT_SUCCESS(tc, "write test data", rv);
+ apr_file_close(f);
+
+ rv = apr_file_open(&f, fname, APR_FOPEN_READ, APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "re-open test file", rv);
+
+ memset(buf, 0, sizeof(buf));
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ APR_ASSERT_SUCCESS(tc, "read first line", rv);
+ ABTS_STR_EQUAL(tc, "a\n", buf);
+
+ memset(buf, 0, sizeof(buf));
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ APR_ASSERT_SUCCESS(tc, "read second line", rv);
+ ABTS_STR_EQUAL(tc, "b\n", buf);
+
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ ABTS_STR_EQUAL(tc, "", buf);
+ /* Calling gets after EOF should return EOF. */
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ ABTS_STR_EQUAL(tc, "", buf);
+ apr_file_close(f);
+}
+
+static void test_gets_small_buf(abts_case *tc, void *data)
+{
+ apr_status_t rv;
+ apr_file_t *f;
+ const char *fname = "data/testgets_small_buf.txt";
+ char buf[2];
+
+ apr_file_remove(fname, p);
+
+ rv = apr_file_open(&f, fname, APR_FOPEN_CREATE | APR_FOPEN_WRITE,
+ APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "open test file", rv);
+ rv = apr_file_puts("ab\n", f);
+ apr_file_close(f);
+
+ rv = apr_file_open(&f, fname, APR_FOPEN_READ, APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "re-open test file", rv);
+ /* Buffer is too small to hold the full line, test that gets properly
+ * returns the line content character by character.
+ */
+ memset(buf, 0, sizeof(buf));
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ APR_ASSERT_SUCCESS(tc, "read first chunk", rv);
+ ABTS_STR_EQUAL(tc, "a", buf);
+
+ memset(buf, 0, sizeof(buf));
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ APR_ASSERT_SUCCESS(tc, "read second chunk", rv);
+ ABTS_STR_EQUAL(tc, "b", buf);
+
+ memset(buf, 0, sizeof(buf));
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ APR_ASSERT_SUCCESS(tc, "read third chunk", rv);
+ ABTS_STR_EQUAL(tc, "\n", buf);
+
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ ABTS_STR_EQUAL(tc, "", buf);
+ /* Calling gets after EOF should return EOF. */
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ ABTS_STR_EQUAL(tc, "", buf);
+ apr_file_close(f);
+}
+
+static void test_gets_ungetc(abts_case *tc, void *data)
+{
+ apr_status_t rv;
+ apr_file_t *f;
+ const char *fname = "data/testgets_ungetc.txt";
+ char buf[256];
+
+ apr_file_remove(fname, p);
+
+ rv = apr_file_open(&f, fname, APR_FOPEN_CREATE | APR_FOPEN_WRITE,
+ APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "open test file", rv);
+ rv = apr_file_puts("a\n", f);
+ APR_ASSERT_SUCCESS(tc, "write test data", rv);
+ apr_file_close(f);
+
+ rv = apr_file_open(&f, fname, APR_FOPEN_READ, APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "re-open test file", rv);
+
+ rv = apr_file_ungetc('b', f);
+ APR_ASSERT_SUCCESS(tc, "call ungetc", rv);
+ memset(buf, 0, sizeof(buf));
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ APR_ASSERT_SUCCESS(tc, "read line", rv);
+ ABTS_STR_EQUAL(tc, "ba\n", buf);
+
+ rv = apr_file_ungetc('\n', f);
+ APR_ASSERT_SUCCESS(tc, "call ungetc with EOL", rv);
+ memset(buf, 0, sizeof(buf));
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ APR_ASSERT_SUCCESS(tc, "read line", rv);
+ ABTS_STR_EQUAL(tc, "\n", buf);
+
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ ABTS_STR_EQUAL(tc, "", buf);
+ /* Calling gets after EOF should return EOF. */
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ ABTS_STR_EQUAL(tc, "", buf);
+ apr_file_close(f);
+}
+
+static void test_gets_buffered_big(abts_case *tc, void *data)
+{
+ apr_status_t rv;
+ apr_file_t *f;
+ const char *fname = "data/testgets_buffered_big.txt";
+ char hugestr[APR_BUFFERSIZE + 2];
+ char buf[APR_BUFFERSIZE + 2];
+
+ apr_file_remove(fname, p);
+
+ rv = apr_file_open(&f, fname, APR_FOPEN_CREATE | APR_FOPEN_WRITE,
+ APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "open test file", rv);
+ /* Test an edge case with a buffered file and the line that exceeds
+ * the default buffer size by 1 (the line itself fits into the buffer,
+ * but the line + EOL does not).
+ */
+ memset(hugestr, 'a', sizeof(hugestr));
+ hugestr[sizeof(hugestr) - 2] = '\n';
+ hugestr[sizeof(hugestr) - 1] = '\0';
+ rv = apr_file_puts(hugestr, f);
+ APR_ASSERT_SUCCESS(tc, "write first line", rv);
+ rv = apr_file_puts("b\n", f);
+ APR_ASSERT_SUCCESS(tc, "write second line", rv);
+ apr_file_close(f);
+
+ rv = apr_file_open(&f, fname, APR_FOPEN_READ | APR_FOPEN_BUFFERED,
+ APR_FPROT_OS_DEFAULT, p);
+ APR_ASSERT_SUCCESS(tc, "re-open test file", rv);
+
+ memset(buf, 0, sizeof(buf));
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ APR_ASSERT_SUCCESS(tc, "read first line", rv);
+ ABTS_STR_EQUAL(tc, hugestr, buf);
+
+ memset(buf, 0, sizeof(buf));
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ APR_ASSERT_SUCCESS(tc, "read second line", rv);
+ ABTS_STR_EQUAL(tc, "b\n", buf);
+
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ ABTS_STR_EQUAL(tc, "", buf);
+ /* Calling gets after EOF should return EOF. */
+ rv = apr_file_gets(buf, sizeof(buf), f);
+ ABTS_INT_EQUAL(tc, APR_EOF, rv);
+ ABTS_STR_EQUAL(tc, "", buf);
apr_file_close(f);
}
@@ -1286,6 +1498,11 @@ abts_suite *testfile(abts_suite *suite)
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_gets_empty, NULL);
+ abts_run_test(suite, test_gets_multiline, NULL);
+ abts_run_test(suite, test_gets_small_buf, NULL);
+ abts_run_test(suite, test_gets_ungetc, NULL);
+ abts_run_test(suite, test_gets_buffered_big, NULL);
abts_run_test(suite, test_puts, NULL);
abts_run_test(suite, test_writev, NULL);
abts_run_test(suite, test_writev_full, NULL);