From ccdb26bf9ee3520a8d25f51f55da4a9d3f04d569 Mon Sep 17 00:00:00 2001 From: Jan Ciolek Date: Wed, 28 Jun 2023 09:11:02 +0200 Subject: [PATCH 1/2] statements/cas_request: fix crash on empty clustering range in LWT LWT queries with empty clustering range used to cause a crash. For example in: ```cql UPDATE tab SET r = 9000 WHERE p = 1 AND c = 2 AND c = 2000 IF r = 3 ``` The range of `c` is empty - there are no valid values. This caused a segfault when accessing the `first` range: ```c++ op.ranges.front() ``` To fix it let's throw en exception when the clustering range is empty. Cassandra also rejects queries with `c = 1 AND c = 2`. There's also a check for empty partition range, as it used to crash in the past, can't really hurt to add it. Signed-off-by: Jan Ciolek --- cql3/statements/cas_request.cc | 6 ++++++ test/cql-pytest/test_lwt.py | 1 - 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/cql3/statements/cas_request.cc b/cql3/statements/cas_request.cc index 5d1df9214a5a..94d1915dda63 100644 --- a/cql3/statements/cas_request.cc +++ b/cql3/statements/cas_request.cc @@ -123,6 +123,9 @@ std::optional cas_request::apply(foreign_ptrvalue().key().value(); // We must ignore statement clustering column restriction when // choosing a row to check the conditions. If there is no @@ -134,6 +137,9 @@ cas_request::old_row cas_request::find_old_row(const cas_row_update& op) const { // CREATE TABLE t(p int, c int, s int static, v int, PRIMARY KEY(p, c)); // INSERT INTO t(p, s) VALUES(1, 1); // UPDATE t SET v=1 WHERE p=1 AND c=1 IF s=1; + if (op.ranges.empty()) { + throw exceptions::invalid_request_exception("Empty clustering range"); + } const clustering_key& ckey = op.ranges.front().start() ? op.ranges.front().start()->value() : empty_ckey; auto row = _rows.find_row(pkey, ckey); auto ckey_ptr = &ckey; diff --git a/test/cql-pytest/test_lwt.py b/test/cql-pytest/test_lwt.py index c9a8c1bcf30f..cad3fe031482 100644 --- a/test/cql-pytest/test_lwt.py +++ b/test/cql-pytest/test_lwt.py @@ -73,7 +73,6 @@ def test_lwt_empty_partition_range(cql, table1): # Generate an LWT update where there is no value for the clustering key, # as the WHERE restricts it using `c = 2 AND c = 3`. # Such queries are rejected. -@pytest.mark.skip(reason="crashes scylla, see issue #13129") def test_lwt_empty_clustering_range(cql, table1): with pytest.raises(InvalidRequest): cql.execute(f"UPDATE {table1} SET r = 9000 WHERE p = 1 AND c = 2 AND c = 2000 IF r = 3") From bfbc3d70b7672dd483fc592d87873b9d5c8efe07 Mon Sep 17 00:00:00 2001 From: Jan Ciolek Date: Wed, 28 Jun 2023 09:38:27 +0200 Subject: [PATCH 2/2] modification_statement: reject conditional statements with empty clustering key `modification_statement::execute_with_condition` validates that a query with an IF condition can be executed correctly. There's already a check for empty partition key ranges, but there was no check for empty clustering ranges. Let's add a check for the clustering ranges as well, they're not allowed to be empty. After this change Scylla outputs the same type of message for empty partition and clustering ranges, which improves UX. Signed-off-by: Jan Ciolek --- cql3/statements/modification_statement.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cql3/statements/modification_statement.cc b/cql3/statements/modification_statement.cc index 6c60555c87b5..057653beb7e4 100644 --- a/cql3/statements/modification_statement.cc +++ b/cql3/statements/modification_statement.cc @@ -318,6 +318,10 @@ modification_statement::execute_with_condition(query_processor& qp, service::que throw exceptions::invalid_request_exception(format("Unrestricted partition key in a conditional {}", type.is_update() ? "update" : "deletion")); } + if (ranges.empty()) { + throw exceptions::invalid_request_exception(format("Unrestricted clustering key in a conditional {}", + type.is_update() ? "update" : "deletion")); + } auto request = seastar::make_shared(s, std::move(keys)); // cas_request can be used for batches as well single statements; Here we have just a single