Repository: qpid-proton Updated Branches: refs/heads/master 19ceafa52 -> 95752d27f
PROTON-1831: [c] reject duplicate link attach with connection error. Fixes the server side part of PROTON-1831 - transport error if a duplicate link name is attached. Client side problem remains: pni_process does not respect the order of individual open/close calls, and it is possible to generate an invalid sequence that attempts a duplicate attach when it should not. This commit includes a test that illustrates the problem, the test runs but status is ignored for now. Project: http://git-wip-us.apache.org/repos/asf/qpid-proton/repo Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/95752d27 Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/95752d27 Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/95752d27 Branch: refs/heads/master Commit: 95752d27f65a210f17eaf4ca59a1b4df67c5dafd Parents: 19ceafa Author: Alan Conway <acon...@redhat.com> Authored: Thu Apr 26 10:21:15 2018 -0400 Committer: Alan Conway <acon...@redhat.com> Committed: Thu Apr 26 10:21:15 2018 -0400 ---------------------------------------------------------------------- c/src/core/transport.c | 11 +++-- c/tests/connection_driver.c | 94 +++++++++++++++++++++++++++++++++++++++- tools/python/proctest.py | 2 +- 3 files changed, 101 insertions(+), 6 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/95752d27/c/src/core/transport.c ---------------------------------------------------------------------- diff --git a/c/src/core/transport.c b/c/src/core/transport.c index 013712a..e0a2b5c 100644 --- a/c/src/core/transport.c +++ b/c/src/core/transport.c @@ -1253,7 +1253,7 @@ int pn_do_begin(pn_transport_t *transport, uint8_t frame_type, uint16_t channel, return 0; } -pn_link_t *pn_find_link(pn_session_t *ssn, pn_bytes_t name, bool is_sender) +static pn_link_t *pni_find_link(pn_session_t *ssn, pn_bytes_t name, bool is_sender) { pn_endpoint_type_t type = is_sender ? SENDER : RECEIVER; @@ -1358,8 +1358,13 @@ int pn_do_attach(pn_transport_t *transport, uint8_t frame_type, uint16_t channel if (strheap) free(strheap); return PN_EOS; } - pn_link_t *link = pn_find_link(ssn, name, is_sender); - if (!link) { + pn_link_t *link = pni_find_link(ssn, name, is_sender); + if (link && (int32_t)link->state.remote_handle >= 0) { + pn_do_error(transport, "amqp:invalid-field", "link name already attached: %s", strname); + if (strheap) free(strheap); + return PN_EOS; + } + if (!link) { /* Make a new link for the attach */ if (is_sender) { link = (pn_link_t *) pn_sender(ssn, strname); } else { http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/95752d27/c/tests/connection_driver.c ---------------------------------------------------------------------- diff --git a/c/tests/connection_driver.c b/c/tests/connection_driver.c index 5997f52..c8e9dbc 100644 --- a/c/tests/connection_driver.c +++ b/c/tests/connection_driver.c @@ -48,6 +48,24 @@ static pn_event_type_t open_handler(test_handler_t *th, pn_event_t *e) { return PN_EVENT_NONE; } +/* Like open_handler but also reply to REMOTE_CLOSE */ +static pn_event_type_t open_close_handler(test_handler_t *th, pn_event_t *e) { + switch (pn_event_type(e)) { + case PN_CONNECTION_REMOTE_CLOSE: + pn_connection_open(pn_event_connection(e)); + break; + case PN_SESSION_REMOTE_CLOSE: + pn_session_open(pn_event_session(e)); + break; + case PN_LINK_REMOTE_CLOSE: + pn_link_close(pn_event_link(e)); + break; + default: + return open_handler(th, e); + } + return PN_EVENT_NONE; +} + /* Handler that returns control on PN_DELIVERY and stores the delivery on the handler */ static pn_event_type_t delivery_handler(test_handler_t *th, pn_event_t *e) { switch (pn_event_type(e)) { @@ -407,8 +425,6 @@ static void set_capacity_and_max_frame( test_connection_drivers_run(client, server); } -#define MAX_WINDOW (2147483647) -#define MAX_FRAME (4294967295) /* Test different settings for max-frame, outgoing-window, incoming-capacity */ static void test_session_flow_control(test_t *t) { test_connection_driver_t client, server; @@ -442,6 +458,76 @@ static void test_session_flow_control(test_t *t) { test_connection_drivers_destroy(&client, &server); } +/* Regression test for https://issues.apache.org/jira/browse/PROTON-1832. + Make sure we error on attempt to re-attach an already-attached link name. + No crash or memory error. +*/ +static void test_duplicate_link_server(test_t *t) { + test_connection_driver_t client, server; + test_connection_drivers_init(t, &client, open_close_handler, &server, open_close_handler); + pn_connection_open(client.driver.connection); + pn_session_t *ssn = pn_session(client.driver.connection); + pn_session_open(ssn); + + /* Set up link "x" */ + pn_link_t *x = pn_sender(ssn, "xxx"); + pn_link_open(x); + test_connection_drivers_run(&client, &server); + test_handler_clear(&client.handler, 0); + test_handler_clear(&server.handler, 0); + /* Free (but don't close) the link and open a new one to generate the invalid double-attach */ + pn_link_free(x); + pn_link_open(pn_sender(ssn, "xxx")); + test_connection_drivers_run(&client, &server); + + TEST_COND_NAME(t, "amqp:invalid-field", pn_transport_condition(server.driver.transport)); + TEST_COND_DESC(t, "xxx", pn_transport_condition(server.driver.transport)); + + TEST_COND_NAME(t, "amqp:invalid-field", pn_connection_remote_condition(client.driver.connection)); + TEST_COND_DESC(t, "xxx", pn_connection_remote_condition(client.driver.connection)); + + /* Freeing the link at this point is allowed but caused a crash in transport_unbind with the bug */ + pn_link_free(server.handler.link); + test_connection_drivers_destroy(&client, &server); +} + +/* Regression test for https://issues.apache.org/jira/browse/PROTON-1832. + Make sure the client does not generate an illegal "attach; attach; detach" sequence + from a legal "pn_link_open(); pn_link_close(); pn_link_open()" sequence +*/ +static void test_duplicate_link_client(test_t *t) { + /* Set up the initial link */ + test_connection_driver_t client, server; + test_connection_drivers_init(t, &client, open_close_handler, &server, open_close_handler); + pn_connection_open(client.driver.connection); + pn_session_t *ssn = pn_session(client.driver.connection); + pn_session_open(ssn); + pn_link_t *x = pn_sender(ssn, "x"); + pn_link_open(x); + test_connection_drivers_run(&client, &server); + test_handler_clear(&client.handler, 0); + test_handler_clear(&server.handler, 0); + + /* Close the link and open a new link with same name in the same batch of events. */ + pn_link_close(x); + pn_link_open(pn_sender(ssn, "x")); + test_connection_drivers_run(&client, &server); + + TEST_HANDLER_EXPECT(&server.handler, + PN_LINK_REMOTE_CLOSE, PN_LINK_LOCAL_CLOSE, PN_TRANSPORT, + PN_LINK_INIT, PN_LINK_REMOTE_OPEN, PN_LINK_LOCAL_OPEN, PN_TRANSPORT, + 0); + TEST_COND_EMPTY(t, pn_transport_condition(server.driver.transport)); + + test_connection_drivers_run(&client, &server); + TEST_HANDLER_EXPECT(&client.handler, + PN_LINK_LOCAL_CLOSE, PN_TRANSPORT, PN_LINK_REMOTE_CLOSE, + PN_LINK_INIT, PN_LINK_LOCAL_OPEN, PN_TRANSPORT, PN_LINK_REMOTE_OPEN, + 0); + TEST_COND_EMPTY(t, pn_connection_remote_condition(client.driver.connection)); + test_connection_drivers_destroy(&client, &server); +} + int main(int argc, char **argv) { int failed = 0; RUN_ARGV_TEST(failed, t, test_message_transfer(&t)); @@ -449,5 +535,9 @@ int main(int argc, char **argv) { RUN_ARGV_TEST(failed, t, test_message_abort(&t)); RUN_ARGV_TEST(failed, t, test_message_abort_mixed(&t)); RUN_ARGV_TEST(failed, t, test_session_flow_control(&t)); + RUN_ARGV_TEST(failed, t, test_duplicate_link_server(&t)); + fprintf(stderr, "\n==== Following tests are expected to fail ====\n"); + int ignore = 0; /* run but don't fail the build */ + RUN_ARGV_TEST(ignore, t, test_duplicate_link_client(&t)); return failed; } http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/95752d27/tools/python/proctest.py ---------------------------------------------------------------------- diff --git a/tools/python/proctest.py b/tools/python/proctest.py index 2b3a04f..6445f95 100644 --- a/tools/python/proctest.py +++ b/tools/python/proctest.py @@ -31,7 +31,7 @@ from copy import copy import platform from os.path import dirname as dirname -DEFAULT_TIMEOUT=10 +DEFAULT_TIMEOUT=30 # Valgrind can be very slow. class ProcError(Exception): """An exception that displays failed process output""" --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@qpid.apache.org For additional commands, e-mail: commits-h...@qpid.apache.org
