Untyped result sets may cause segfaults when parsing disengaged optionals

[psarna]

In untyped_result_set.hh, function get_view silently assumes that the optional value kept in its internal map is always engaged. It's not always the case, since the optional values come from the tables (they are often a result of the SELECT statement), and thus they shouldn't assume that the values are always present.
For instance, system_keyspace::load_view_build_progress can fail when a particular column - "first_token" - is not present in the system.view_builds_in_progress table - which is entirely possible, since we cannot assume any particular contents inside a CQL table.
Current code creates an incorrect bytes_view object when it's given a disengaged optional, and it may lead to unexpected segfaults later. It's better to check if the optional value is engaged, and throw if it's not.

[avikivity]

@psarna should this be backported? I don't think we have a trigger.

[psarna]

We don't have a trigger, but corrupt system tables would suffice. I think I'll backport, since it's a bug (segfault instead of graciously failing with a recoverable exception) and the backporting cost is cheap.

[psarna]

Neither 4.1 nor 4.2 backports cleanly, because CDC relied on dereferencing disengaged optionals somewhere, and it shows in cdc_static_and_clustered_rows_test... it's definitely fixed already in master, so I guess that this fix, whatever it is, should be backported first? Or is CDC considered experimental in 4.1/4.2, which means no backports? @haaawk any idea which patch fixed the CDC issue?

[haaawk]

Do you know which optional was the problem in CDC, @psarna ?

[psarna]

Not precisely, this is the output of the failed test:

 -- generates 3 rows: preimage(static), delta(static), delta(clustering)
 update ks.t set vs = 2, vc = 2 where pk = 0 and ck = 0;
 {
-	"status" : "ok"
+	"message" : "std::bad_optional_access (bad optional access)",
+	"status" : "error"
 }
 -- generates 4 rows: preimage(static), preimage(clustering), delta(static), delta(clustering)
 update ks.t set vs = 3, vc = 3 where pk = 0 and ck = 0;
 {
-	"status" : "ok"
+	"message" : "std::bad_optional_access (bad optional access)",
+	"status" : "error"
 }
 select "cdc$batch_seq_no", "cdc$operation", ck, vs, vc from ks.t_scylla_cdc_log where pk = 0 and ck = 0 allow filtering;
 {
-	"rows" : 
-	[
-		{
-			"cdc$batch_seq_no" : "2",
-			"cdc$operation" : "1",
-			"ck" : "0",
-			"pk" : "0",
-			"vc" : "2"
-		},
-		{
-			"cdc$batch_seq_no" : "2",
-			"cdc$operation" : "0",
-			"ck" : "0",
-			"pk" : "0",
-			"vc" : "2"
-		},
-		{
-			"cdc$batch_seq_no" : "3",
-			"cdc$operation" : "1",
-			"ck" : "0",
-			"pk" : "0",
-			"vc" : "3"
-		}
-	]
+	"rows" : null
 }

[haaawk]

Does the test fail every time or are there some special conditions?

[psarna]

every single time on 4.1/4.2, never on master

[haaawk]

Could you try removing "vs = 2" and "vs = 3" from updates and see if this fixes he exception?

[psarna]

Well, it definitely fixes std::bad_optional_access (bad optional access), because these were the only two statements that produced the error, but the test still fails after removing these updates (as expected).

[haaawk]

Cool. That narrows down the search to the code related to static row. If we want to find the patch that fixed the problem.

[eliransin]

@psarna / @haaawk what should be done to find this patch? (besides brute force bisection of course 🙂 )
Can one of you help with that?

[haaawk]

Do we have to do anything? It seems that the patch is fine on master is backporting necessary?

[eliransin]

@haaawk, according to this #6915 (comment) I was under the impression
that we don't even know what patch is to be backported (cdc patch that fixes relying on disengaged optionals mentioned by @psarna - #6915 (comment))
If I'm wrong and we do know what this patch is, can you please reference it here so I can see if it is a viable candidate for
backport?

[haaawk]

You're right, @eliransin - we don't know which CDC patch affects the behavior. I was referring to the fact that it's not clear we want to backport this fix at all. @avikivity wrote:

@psarna should this be backported? I don't think we have a trigger.

and then @psarna wrote:

We don't have a trigger [...] and the backporting cost is cheap

This left me with the impression there's no need for backporting and spending time on looking for this CDC patch wouldn't be the best use of our time when we can do something more useful instead.

[eliransin]

Thanks @haaawk for clarification. I will update if we have a trigger.

[avikivity]

Looks like there was no trigger, so removing the backport candidate label.