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);
