From: Willy Tarreau Date: Mon, 20 Apr 2015 11:26:17 +0000 (+0200) Subject: BUG/MAJOR: tcp/http: fix current_rule assignment when restarting over a ruleset X-Git-Tag: v1.6-dev2~208 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=152b81e7b2565862956af883820d4f79177d0651;p=thirdparty%2Fhaproxy.git BUG/MAJOR: tcp/http: fix current_rule assignment when restarting over a ruleset Commit bc4c1ac ("MEDIUM: http/tcp: permit to resume http and tcp custom actions") introduced the ability to interrupt and restart processing in the middle of a TCP/HTTP ruleset. But it doesn't do it in a consistent way : it checks current_rule_list, immediately dereferences current_rule, which is only set in certain cases and never cleared. So that broke the tcp-request content rules when the processing was interrupted due to missing data, because current_rule was not yet set (segfault) or could have been inherited from another ruleset if it was used in a backend (random behaviour). The proper way to do it is to always set current_rule before dereferencing it. But we don't want to set it for all rules because we don't want any action to provide a checkpointing mechanism. So current_rule is set to NULL before entering the loop, and only used if not NULL and if current_rule_list matches the current list. This way they both serve as a guard for the other one. This fix also makes the current rule point to the rule instead of its list element, as it's much easier to manipulate. No backport is needed, this is 1.6-specific. --- diff --git a/include/types/stream.h b/include/types/stream.h index be285683d6..4203af8db6 100644 --- a/include/types/stream.h +++ b/include/types/stream.h @@ -156,7 +156,7 @@ struct stream { /* These two pointers are used to resume the execution of the rule lists. */ struct list *current_rule_list; /* this is used to store the current executed rule list. */ - struct list *current_rule; /* this is used to store the current rule to be resumed. */ + void *current_rule; /* this is used to store the current rule to be resumed. */ struct hlua hlua; /* lua runtime context */ }; diff --git a/src/proto_http.c b/src/proto_http.c index 4db982aeea..377160b4ba 100644 --- a/src/proto_http.c +++ b/src/proto_http.c @@ -3359,11 +3359,14 @@ http_req_get_intercept_rule(struct proxy *px, struct list *rules, struct stream * and never in the ACL or converters. In this case, we initialise the * current rule, and go to the action execution point. */ - if (s->current_rule_list == rules) { - rule = LIST_ELEM(s->current_rule, typeof(rule), list); - goto resume_execution; + if (s->current_rule) { + rule = s->current_rule; + s->current_rule = NULL; + if (s->current_rule_list == rules) + goto resume_execution; } s->current_rule_list = rules; + list_for_each_entry(rule, rules, list) { if (rule->action >= HTTP_REQ_ACT_MAX) continue; @@ -3383,7 +3386,6 @@ http_req_get_intercept_rule(struct proxy *px, struct list *rules, struct stream } resume_execution: - switch (rule->action) { case HTTP_REQ_ACT_ALLOW: return HTTP_RULE_RES_STOP; @@ -3566,7 +3568,7 @@ resume_execution: case HTTP_REQ_ACT_CUSTOM_CONT: if (!rule->action_ptr(rule, px, s)) { - s->current_rule = &rule->list; + s->current_rule = rule; return HTTP_RULE_RES_YIELD; } break; @@ -3637,11 +3639,14 @@ http_res_get_intercept_rule(struct proxy *px, struct list *rules, struct stream * and never in the ACL or converters. In this case, we initialise the * current rule, and go to the action execution point. */ - if (s->current_rule_list == rules) { - rule = LIST_ELEM(s->current_rule, typeof(rule), list); - goto resume_execution; + if (s->current_rule) { + rule = s->current_rule; + s->current_rule = NULL; + if (s->current_rule_list == rules) + goto resume_execution; } s->current_rule_list = rules; + list_for_each_entry(rule, rules, list) { if (rule->action >= HTTP_RES_ACT_MAX) continue; @@ -3661,7 +3666,6 @@ http_res_get_intercept_rule(struct proxy *px, struct list *rules, struct stream } resume_execution: - switch (rule->action) { case HTTP_RES_ACT_ALLOW: return HTTP_RULE_RES_STOP; /* "allow" rules are OK */ @@ -3814,7 +3818,7 @@ resume_execution: case HTTP_RES_ACT_CUSTOM_CONT: if (!rule->action_ptr(rule, px, s)) { - s->current_rule = &rule->list; + s->current_rule = rule; return HTTP_RULE_RES_YIELD; } break; diff --git a/src/proto_tcp.c b/src/proto_tcp.c index 17ec22e1bc..efa9158bfc 100644 --- a/src/proto_tcp.c +++ b/src/proto_tcp.c @@ -1125,11 +1125,14 @@ int tcp_inspect_request(struct stream *s, struct channel *req, int an_bit) * and never in the ACL or converters. In this case, we initialise the * current rule, and go to the action execution point. */ - if (s->current_rule_list == &s->be->tcp_req.inspect_rules) { - rule = LIST_ELEM(s->current_rule, typeof(rule), list); - goto resume_execution; + if (s->current_rule) { + rule = s->current_rule; + s->current_rule = NULL; + if (s->current_rule_list == &s->be->tcp_req.inspect_rules) + goto resume_execution; } s->current_rule_list = &s->be->tcp_req.inspect_rules; + list_for_each_entry(rule, &s->be->tcp_req.inspect_rules, list) { enum acl_test_res ret = ACL_TEST_PASS; @@ -1144,9 +1147,7 @@ int tcp_inspect_request(struct stream *s, struct channel *req, int an_bit) } if (ret) { - resume_execution: - /* we have a matching rule. */ if (rule->action == TCP_ACT_REJECT) { channel_abort(req); @@ -1216,7 +1217,7 @@ resume_execution: else { /* Custom keywords. */ if (rule->action_ptr(rule, s->be, s) == 0) { - s->current_rule = &rule->list; + s->current_rule = rule; goto missing_data; } @@ -1283,11 +1284,14 @@ int tcp_inspect_response(struct stream *s, struct channel *rep, int an_bit) * and never in the ACL or converters. In this case, we initialise the * current rule, and go to the action execution point. */ - if (s->current_rule_list == &s->be->tcp_rep.inspect_rules) { - rule = LIST_ELEM(s->current_rule, typeof(rule), list); - goto resume_execution; + if (s->current_rule) { + rule = s->current_rule; + s->current_rule = NULL; + if (s->current_rule_list == &s->be->tcp_rep.inspect_rules) + goto resume_execution; } s->current_rule_list = &s->be->tcp_rep.inspect_rules; + list_for_each_entry(rule, &s->be->tcp_rep.inspect_rules, list) { enum acl_test_res ret = ACL_TEST_PASS; @@ -1306,9 +1310,7 @@ int tcp_inspect_response(struct stream *s, struct channel *rep, int an_bit) } if (ret) { - resume_execution: - /* we have a matching rule. */ if (rule->action == TCP_ACT_REJECT) { channel_abort(rep); @@ -1336,7 +1338,7 @@ resume_execution: /* Custom keywords. */ if (!rule->action_ptr(rule, s->be, s)) { channel_dont_close(rep); - s->current_rule = &rule->list; + s->current_rule = rule; return 0; }