Author: ivan
Date: Wed Sep 13 08:28:22 2017
New Revision: 1808209

URL: http://svn.apache.org/viewvc?rev=1808209&view=rev
Log:
Merge:
 * r1804534, r1804543, r1804553
   Fix error handling when reading the outgoing request's body by not
   ignoring such errors in some cases.
   Justification:
     Ignoring errors can cause invalid or undefined behavior further
     down the line.
   Branch:
     ^/serf/branches/1.3.x-fix-outgoing-request-err
   Notes:
     - On trunk, this issue has been fixed in the process of adding
       the pump.c layer.
     - An example of a condition where the error could be incorrectly ignored
       is Subversion's commit, if the error occurs when reading the body of
       an outgoing PUT request.
     - The added test doesn't fail without the fix, but is merely added to
       cover the related code.  (See the r1804541 log message for details.)
   Votes:
     +1: kotkov, ivan, rhuijben

Modified:
    serf/branches/1.3.x/   (props changed)
    serf/branches/1.3.x/STATUS
    serf/branches/1.3.x/outgoing.c
    serf/branches/1.3.x/test/test_context.c
    serf/branches/1.3.x/test/test_serf.h
    serf/branches/1.3.x/test/test_util.c

Propchange: serf/branches/1.3.x/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Wed Sep 13 08:28:22 2017
@@ -1,4 +1,4 @@
-/serf/branches/1.3.x:1699925,1699931
+/serf/branches/1.3.x-fix-outgoing-request-err:1804540-1808208
 /serf/branches/multiple_ssl_impls:1699382
 /serf/branches/windows-sspi:1698866-1698877
-/serf/trunk:1699516-1699518,1699520-1699522,1699528,1699530-1699535,1699537,1699539-1699543,1699548-1699549,1699553,1699555-1699556,1699559-1699560,1699563-1699565,1699567-1699570,1699572-1699573,1699578-1699580,1699582-1699597,1699599-1699602,1699607,1699610,1699613,1699615-1699618,1699622-1699623,1699626-1699627,1699633,1699637,1699642,1699645,1699647,1699649-1699650,1699652,1699654-1699655,1699659-1699665,1699671,1699674,1699680-1699683,1699687-1699688,1699690,1699692-1699694,1699698-1699700,1699702,1699707-1699708,1699712-1699716,1699720,1699724,1699728,1699730,1699733,1699762,1699770,1699773,1699777,1699780-1699781,1699791,1699798,1699800-1699801,1699817,1699819,1699838,1699843,1699846,1699850,1699852,1699858-1699859,1699861,1699873,1699881,1699884,1699902-1699903,1699906,1699924,1699926-1699927,1699930,1699932,1699936-1699937,1699941,1699944,1699948-1699950,1699954,1699957,1699964,1699973,1699975,1699985-1699987,1699993-1699994,1700031,1700062,1700128,1700149,1700234,1700236,1
 
700246,1700270,1700650,1700830,1702096,1702221,1702264,1703624,1704725,1708849,1709155-1709156,1709296,1748673,1757829,1758190,1758193
+/serf/trunk:1699516-1699518,1699520-1699522,1699528,1699530-1699535,1699537,1699539-1699543,1699548-1699549,1699553,1699555-1699556,1699559-1699560,1699563-1699565,1699567-1699570,1699572-1699573,1699578-1699580,1699582-1699597,1699599-1699602,1699607,1699610,1699613,1699615-1699618,1699622-1699623,1699626-1699627,1699633,1699637,1699642,1699645,1699647,1699649-1699650,1699652,1699654-1699655,1699659-1699665,1699671,1699674,1699680-1699683,1699687-1699688,1699690,1699692-1699694,1699698-1699700,1699702,1699707-1699708,1699712-1699716,1699720,1699724,1699728,1699730,1699733,1699762,1699770,1699773,1699777,1699780-1699781,1699791,1699798,1699800-1699801,1699817,1699819,1699838,1699843,1699846,1699850,1699852,1699858-1699859,1699861,1699873,1699881,1699884,1699902-1699903,1699906,1699924,1699926-1699927,1699930,1699932,1699936-1699937,1699941,1699944,1699948-1699950,1699954,1699957,1699964,1699973,1699975,1699985-1699987,1699993-1699994,1700031,1700062,1700128,1700149,1700234,1700236,1
 
700246,1700270,1700650,1700830,1702096,1702221,1702264,1703624,1704725,1708849,1709155-1709156,1709296,1748673,1757829,1758190,1758193,1804534

Modified: serf/branches/1.3.x/STATUS
URL: 
http://svn.apache.org/viewvc/serf/branches/1.3.x/STATUS?rev=1808209&r1=1808208&r2=1808209&view=diff
==============================================================================
--- serf/branches/1.3.x/STATUS (original)
+++ serf/branches/1.3.x/STATUS Wed Sep 13 08:28:22 2017
@@ -30,25 +30,6 @@ Veto-blocked changes:
 Approved changes:
 =================
 
- * r1804534, r1804543, r1804553
-   Fix error handling when reading the outgoing request's body by not
-   ignoring such errors in some cases.
-   Justification:
-     Ignoring errors can cause invalid or undefined behavior further
-     down the line.
-   Branch:
-     ^/serf/branches/1.3.x-fix-outgoing-request-err
-   Notes:
-     - On trunk, this issue has been fixed in the process of adding
-       the pump.c layer.
-     - An example of a condition where the error could be incorrectly ignored
-       is Subversion's commit, if the error occurs when reading the body of
-       an outgoing PUT request.
-     - The added test doesn't fail without the fix, but is merely added to
-       cover the related code.  (See the r1804541 log message for details.)
-   Votes:
-     +1: kotkov, ivan, rhuijben
-
  * r1804005, r1804008, r1804016
    Return an error for invalid chunk lengths in the dechunk bucket.
    Justification:

Modified: serf/branches/1.3.x/outgoing.c
URL: 
http://svn.apache.org/viewvc/serf/branches/1.3.x/outgoing.c?rev=1808209&r1=1808208&r2=1808209&view=diff
==============================================================================
--- serf/branches/1.3.x/outgoing.c (original)
+++ serf/branches/1.3.x/outgoing.c Wed Sep 13 08:28:22 2017
@@ -815,11 +815,14 @@ static apr_status_t write_to_connection(
            data as available, we probably don't want to read ALL_AVAIL, but
            a lower number, like the size of one or a few TCP packets, the
            available TCP buffer size ... */
+        conn->hit_eof = 0;
         read_status = serf_bucket_read_iovec(ostreamh,
                                              SERF_READ_ALL_AVAIL,
                                              IOV_MAX,
                                              conn->vec,
                                              &conn->vec_len);
+        if (SERF_BUCKET_READ_ERROR(read_status))
+            return read_status;
 
         if (!conn->hit_eof) {
             if (APR_STATUS_IS_EAGAIN(read_status)) {
@@ -841,9 +844,8 @@ static apr_status_t write_to_connection(
                 conn->dirty_conn = 1;
                 conn->ctx->dirty_pollset = 1;
             }
-            else if (read_status && !APR_STATUS_IS_EOF(read_status)) {
-                /* Something bad happened. Propagate any errors. */
-                return read_status;
+            else if (APR_STATUS_IS_EOF(read_status)) {
+                /* Continue. */
             }
         }
 

Modified: serf/branches/1.3.x/test/test_context.c
URL: 
http://svn.apache.org/viewvc/serf/branches/1.3.x/test/test_context.c?rev=1808209&r1=1808208&r2=1808209&view=diff
==============================================================================
--- serf/branches/1.3.x/test/test_context.c (original)
+++ serf/branches/1.3.x/test/test_context.c Wed Sep 13 08:28:22 2017
@@ -397,8 +397,8 @@ static void test_keepalive_limit_one_by_
     CuAssertIntEquals(tc, APR_SUCCESS, status);
 
     for (i = 0 ; i < SEND_REQUESTS ; i++) {
-        create_new_request_with_resp_hdlr(tb, &handler_ctx[i], "GET", "/", i+1,
-                                          handle_response_keepalive_limit);
+        create_new_request_ex(tb, &handler_ctx[i], "GET", "/", i+1,
+                              NULL, handle_response_keepalive_limit);
         /* TODO: don't think this needs to be done in the loop. */
         serf_connection_set_max_outstanding_requests(tb->connection, 1);
     }
@@ -528,8 +528,8 @@ static void test_keepalive_limit_one_by_
     CuAssertIntEquals(tc, APR_SUCCESS, status);
 
     for (i = 0 ; i < SEND_REQUESTS ; i++) {
-        create_new_request_with_resp_hdlr(tb, &handler_ctx[i], "GET", "/", i+1,
-                                          
handle_response_keepalive_limit_burst);
+        create_new_request_ex(tb, &handler_ctx[i], "GET", "/", i+1,
+                              NULL, handle_response_keepalive_limit_burst);
         serf_connection_set_max_outstanding_requests(tb->connection, 1);
     }
 
@@ -2282,6 +2282,51 @@ static void test_ssltunnel_digest_auth(C
     CuAssertTrue(tc, tb->result_flags & TEST_RESULT_AUTHNCB_CALLED);
 }
 
+/* Implements test_request_setup_t */
+static apr_status_t setup_request_err(serf_request_t *request,
+                                      void *setup_baton,
+                                      serf_bucket_t **req_bkt,
+                                      apr_pool_t *pool)
+{
+    static mockbkt_action actions[] = {
+        { 1, "a", APR_SUCCESS },
+        { 1, "", APR_EINVAL }
+    };
+    handler_baton_t *ctx = setup_baton;
+    serf_bucket_alloc_t *alloc;
+    serf_bucket_t *mock_bkt;
+
+    alloc = serf_request_get_alloc(request);
+    mock_bkt = serf_bucket_mock_create(actions, 2, alloc);
+    *req_bkt = serf_request_bucket_request_create(request,
+                                                  ctx->method, ctx->path,
+                                                  mock_bkt, alloc);
+    return APR_SUCCESS;
+}
+
+static void test_outgoing_request_err(CuTest *tc)
+{
+    test_baton_t *tb;
+    handler_baton_t handler_ctx[1];
+    apr_status_t status;
+    apr_pool_t *test_pool = tc->testBaton;
+
+    status = test_http_server_setup(&tb, NULL, 0,
+                                    NULL, 0, 0, NULL,
+                                    test_pool);
+    CuAssertIntEquals(tc, APR_SUCCESS, status);
+
+    create_new_request_ex(tb, &handler_ctx[0], "POST", "/", 1,
+                          setup_request_err, NULL);
+
+    status = test_helper_run_requests_no_check(tc, tb, 1, handler_ctx,
+                                               test_pool);
+    CuAssertIntEquals(tc, APR_EINVAL, status);
+    CuAssertIntEquals(tc, 1, tb->sent_requests->nelts);
+    CuAssertIntEquals(tc, 0, tb->accepted_requests->nelts);
+    CuAssertIntEquals(tc, 0, tb->handled_requests->nelts);
+}
+
 /*****************************************************************************/
 CuSuite *test_context(void)
 {
@@ -2319,6 +2364,7 @@ CuSuite *test_context(void)
     SUITE_ADD_TEST(suite, test_ssltunnel_basic_auth_proxy_has_keepalive_off);
     SUITE_ADD_TEST(suite, 
test_ssltunnel_basic_auth_proxy_close_conn_on_200resp);
     SUITE_ADD_TEST(suite, test_ssltunnel_digest_auth);
+    SUITE_ADD_TEST(suite, test_outgoing_request_err);
 
     return suite;
 }

Modified: serf/branches/1.3.x/test/test_serf.h
URL: 
http://svn.apache.org/viewvc/serf/branches/1.3.x/test/test_serf.h?rev=1808209&r1=1808208&r2=1808209&view=diff
==============================================================================
--- serf/branches/1.3.x/test/test_serf.h (original)
+++ serf/branches/1.3.x/test/test_serf.h Wed Sep 13 08:28:22 2017
@@ -193,6 +193,13 @@ apr_status_t test_https_server_proxy_set
 void *test_setup(void *baton);
 void *test_teardown(void *baton);
 
+/* Simple variant of serf_request_setup_t for tests. */
+typedef apr_status_t (*test_request_setup_t)(
+    serf_request_t *request,
+    void *setup_baton,
+    serf_bucket_t **req_bkt,
+    apr_pool_t *pool);
+
 typedef struct {
     serf_response_acceptor_t acceptor;
     void *acceptor_baton;
@@ -208,6 +215,8 @@ typedef struct {
     const char *path;
     /* Use this for a raw request message */
     const char *request;
+    /* Or this, if more control is needed. */
+    test_request_setup_t request_setup;
     int done;
 
     test_baton_t *tb;
@@ -249,6 +258,7 @@ apr_status_t handle_response(serf_reques
 void setup_handler(test_baton_t *tb, handler_baton_t *handler_ctx,
                    const char *method, const char *path,
                    int req_id,
+                   test_request_setup_t req_setup,
                    serf_response_handler_t handler);
 void create_new_prio_request(test_baton_t *tb,
                              handler_baton_t *handler_ctx,
@@ -259,11 +269,12 @@ void create_new_request(test_baton_t *tb
                         const char *method, const char *path,
                         int req_id);
 void
-create_new_request_with_resp_hdlr(test_baton_t *tb,
-                                  handler_baton_t *handler_ctx,
-                                  const char *method, const char *path,
-                                  int req_id,
-                                  serf_response_handler_t handler);
+create_new_request_ex(test_baton_t *tb,
+                      handler_baton_t *handler_ctx,
+                      const char *method, const char *path,
+                      int req_id,
+                      test_request_setup_t req_setup,
+                      serf_response_handler_t handler);
 
 /* Mock bucket type and constructor */
 typedef struct {

Modified: serf/branches/1.3.x/test/test_util.c
URL: 
http://svn.apache.org/viewvc/serf/branches/1.3.x/test/test_util.c?rev=1808209&r1=1808208&r2=1808209&view=diff
==============================================================================
--- serf/branches/1.3.x/test/test_util.c (original)
+++ serf/branches/1.3.x/test/test_util.c Wed Sep 13 08:28:22 2017
@@ -498,8 +498,13 @@ apr_status_t setup_request(serf_request_
 {
     handler_baton_t *ctx = setup_baton;
     serf_bucket_t *body_bkt;
+    apr_status_t status = APR_SUCCESS;
 
-    if (ctx->request)
+    if (ctx->request_setup)
+    {
+        status = ctx->request_setup(request, setup_baton, req_bkt, pool);
+    }
+    else if (ctx->request)
     {
         /* Create a raw request bucket. */
         *req_bkt = serf_bucket_simple_create(ctx->request, 
strlen(ctx->request),
@@ -533,7 +538,7 @@ apr_status_t setup_request(serf_request_
     *handler = ctx->handler;
     *handler_baton = ctx;
 
-    return APR_SUCCESS;
+    return status;
 }
 
 apr_status_t handle_response(serf_request_t *request,
@@ -577,6 +582,7 @@ apr_status_t handle_response(serf_reques
 void setup_handler(test_baton_t *tb, handler_baton_t *handler_ctx,
                    const char *method, const char *path,
                    int req_id,
+                   test_request_setup_t req_setup,
                    serf_response_handler_t handler)
 {
     handler_ctx->method = method;
@@ -592,6 +598,7 @@ void setup_handler(test_baton_t *tb, han
     handler_ctx->handled_requests = tb->handled_requests;
     handler_ctx->tb = tb;
     handler_ctx->request = NULL;
+    handler_ctx->request_setup = req_setup;
 }
 
 void create_new_prio_request(test_baton_t *tb,
@@ -599,7 +606,7 @@ void create_new_prio_request(test_baton_
                              const char *method, const char *path,
                              int req_id)
 {
-    setup_handler(tb, handler_ctx, method, path, req_id, NULL);
+    setup_handler(tb, handler_ctx, method, path, req_id, NULL, NULL);
     serf_connection_priority_request_create(tb->connection,
                                             setup_request,
                                             handler_ctx);
@@ -610,20 +617,21 @@ void create_new_request(test_baton_t *tb
                         const char *method, const char *path,
                         int req_id)
 {
-    setup_handler(tb, handler_ctx, method, path, req_id, NULL);
+    setup_handler(tb, handler_ctx, method, path, req_id, NULL, NULL);
     serf_connection_request_create(tb->connection,
                                    setup_request,
                                    handler_ctx);
 }
 
 void
-create_new_request_with_resp_hdlr(test_baton_t *tb,
-                                  handler_baton_t *handler_ctx,
-                                  const char *method, const char *path,
-                                  int req_id,
-                                  serf_response_handler_t handler)
+create_new_request_ex(test_baton_t *tb,
+                      handler_baton_t *handler_ctx,
+                      const char *method, const char *path,
+                      int req_id,
+                      test_request_setup_t req_setup,
+                      serf_response_handler_t handler)
 {
-    setup_handler(tb, handler_ctx, method, path, req_id, handler);
+    setup_handler(tb, handler_ctx, method, path, req_id, req_setup, handler);
     serf_connection_request_create(tb->connection,
                                    setup_request,
                                    handler_ctx);


Reply via email to