Skip to content

Commit

Permalink
internal/core/adt: fix index out of bound panic
Browse files Browse the repository at this point in the history 
The main issue here was that when a Vertex was cloned,
in overlay.go, its corresponding closeContext was cloned
first, alongside its group. Normally, the group of a
closeContext associated with a Vertex has its group
pointing to the Conjuncts of this Vertex, ensuring they
are always the same. The copy made in the overlay code,
however, only copied the slice pointers already allocated,
but the two slices would get out of sync when more
conjuncts were added. This caused the index to overrun.

This fix also exposed another issue where the group can
sometimes be empty. Although this is theoretically possible,
the conditions under which this appeared are suspicious.
This CL, therefore, added a TODO to investigate. A simplified
reproducer for this is:

A: v: "a" | "b"
h: [_]: A
h: a: v:  *"a" | string
h: b: v:  *h.a.v | string
h: ["b"]: v: *h.b.v | string

This example also exposes the counter issue, three of which
were added because of this change.

Tests were added in this CL, as they cause a panic otherwise.

Fixes #3597

A further related issue is fixed in a next CL.

Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com>
Change-Id: I61b6aafb2f439793388f9357001d8ec99a30da3f
Reviewed-on: https://gerrithub.io/c/cue-lang/cue/+/1206926
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Reviewed-by: Matthew Sackman <matthew@cue.works>
  • Loading branch information
@mpvl
mpvl committed Jan 9, 2025
1 parent fddb432 commit 96cff7c
Show file tree
Hide file tree
Showing 5 changed files with 202 additions and 18 deletions.
4 changes: 2 additions & 2 deletions cue/testdata/benchmarks/disjunctelim.txtar
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ Allocs: 572
Retain: 0

Unifications: 22
Conjuncts: 825
Conjuncts: 877
Disjuncts: 618
-- diff/-out/evalalpha/stats<==>+out/eval/stats --
diff old new
Expand All @@ -68,7 +68,7 @@ diff old new
-Conjuncts: 866
-Disjuncts: 367
+Unifications: 22
+Conjuncts: 825
+Conjuncts: 877
+Disjuncts: 618
-- out/eval/stats --
Leaks: 0
Expand Down
4 changes: 2 additions & 2 deletions cue/testdata/benchmarks/issue1684.txtar
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ Allocs: 2135
Retain: 0

Unifications: 475
Conjuncts: 4799
Conjuncts: 4826
Disjuncts: 918
-- out/evalalpha --
(struct){
Expand Down Expand Up @@ -153,7 +153,7 @@ diff old new
-Conjuncts: 2480117
-Disjuncts: 1064333
+Unifications: 475
+Conjuncts: 4799
+Conjuncts: 4826
+Disjuncts: 918
-- diff/-out/evalalpha<==>+out/eval --
diff old new
Expand Down
191 changes: 183 additions & 8 deletions cue/testdata/eval/disjunctions.txtar
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,16 +180,40 @@ issue3434: full: {
}
}
}
-- issue3597.cue --
issue3597: t1: {
#Schema: {
_ok: bool
{} | {
_ok: true
let Ok = _ok
{} | {x: Ok}
}
}

[string]: #Schema

foo: bar
bar: {}
}
issue3597: t2: {
ok: true
X: _ | {
let Ok = ok
_ | {x: Ok}
}
foo: X & {X}
}
-- out/eval/stats --
Leaks: 0
Freed: 449
Reused: 433
Allocs: 16
Retain: 26
Leaks: 8
Freed: 561
Reused: 547
Allocs: 22
Retain: 34

Unifications: 210
Conjuncts: 817
Disjuncts: 451
Unifications: 286
Conjuncts: 1125
Disjuncts: 571
-- out/evalalpha --
Errors:
f: 2 errors in empty disjunction:
Expand Down Expand Up @@ -429,6 +453,60 @@ Result:
}
}
}
issue3597: (struct){
t1: (struct){
#Schema: (#struct){ |((#struct){
_ok: (bool){ bool }
}, (#struct){
_ok: (bool){ true }
let Ok#1 = (bool){ true }
}, (#struct){
_ok: (bool){ true }
let Ok#1 = (bool){ true }
x: (bool){ true }
}) }
foo: (#struct){ |((#struct){
_ok: (bool){ bool }
}, (#struct){
_ok: (bool){ true }
let Ok#1 = (bool){ true }
}, (#struct){
_ok: (bool){ true }
let Ok#1 = (bool){ true }
}, (#struct){
_ok: (bool){ true }
let Ok#1multi = 〈0;_ok〉
x: (bool){ true }
}) }
bar: (#struct){ |((#struct){
_ok: (bool){ bool }
}, (#struct){
_ok: (bool){ true }
let Ok#1 = (bool){ true }
}, (#struct){
_ok: (bool){ true }
let Ok#1 = (bool){ true }
x: (bool){ true }
}) }
}
t2: (struct){
ok: (bool){ true }
X: (_){ |((_){ _ }, (_){
_
let Ok#2 = (bool){ true }
}, (struct){
let Ok#2 = (bool){ true }
x: (bool){ true }
}) }
foo: (_){ |((_){ _ }, (_){
_
let Ok#2 = (bool){ true }
}, (struct){
let Ok#2 = (bool){ true }
x: (bool){ true }
}) }
}
}
}
-- diff/-out/evalalpha<==>+out/eval --
diff old new
Expand Down Expand Up @@ -477,6 +555,18 @@ diff old new
}
}
}
@@ -264,6 +255,9 @@
let Ok#1 = (bool){ true }
}, (#struct){
_ok: (bool){ true }
+ let Ok#1 = (bool){ true }
+ }, (#struct){
+ _ok: (bool){ true }
let Ok#1multi = 〈0;_ok〉
x: (bool){ true }
}) }
-- diff/todo/p1 --
issue3597.t1.foo: duplicate disjunct
-- diff/todo/p3 --
Missing error positions.
It is probably okay to show name and val fields with original values, as long
Expand Down Expand Up @@ -730,6 +820,57 @@ Result:
}
}
}
issue3597: (struct){
t1: (struct){
#Schema: (#struct){ |((#struct){
_ok: (bool){ bool }
}, (#struct){
_ok: (bool){ true }
let Ok#1 = (bool){ true }
}, (#struct){
_ok: (bool){ true }
let Ok#1 = (bool){ true }
x: (bool){ true }
}) }
foo: (#struct){ |((#struct){
_ok: (bool){ bool }
}, (#struct){
_ok: (bool){ true }
let Ok#1 = (bool){ true }
}, (#struct){
_ok: (bool){ true }
let Ok#1multi = 〈0;_ok〉
x: (bool){ true }
}) }
bar: (#struct){ |((#struct){
_ok: (bool){ bool }
}, (#struct){
_ok: (bool){ true }
let Ok#1 = (bool){ true }
}, (#struct){
_ok: (bool){ true }
let Ok#1 = (bool){ true }
x: (bool){ true }
}) }
}
t2: (struct){
ok: (bool){ true }
X: (_){ |((_){ _ }, (_){
_
let Ok#2 = (bool){ true }
}, (struct){
let Ok#2 = (bool){ true }
x: (bool){ true }
}) }
foo: (_){ |((_){ _ }, (_){
_
let Ok#2 = (bool){ true }
}, (struct){
let Ok#2 = (bool){ true }
x: (bool){ true }
}) }
}
}
}
-- out/compile --
--- in.cue
Expand Down Expand Up @@ -1041,3 +1182,37 @@ Result:
}
}
}
--- issue3597.cue
{
issue3597: {
t1: {
#Schema: {
_ok: bool
({}|{
_ok: true
let Ok#1 = 〈0;_ok〉
({}|{
x: 〈1;let Ok#1〉
})
})
}
[string]: 〈0;#Schema〉
foo: 〈0;bar〉
bar: {}
}
}
issue3597: {
t2: {
ok: true
X: (_|{
let Ok#2 = 〈1;ok〉
(_|{
x: 〈1;let Ok#2〉
})
})
foo: (〈0;X〉 & {
〈1;X〉
})
}
}
}
1 change: 1 addition & 0 deletions internal/core/adt/eval_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ var skipDebugDepErrors = map[string]int{
"disjunctions/elimination": 11,
"disjunctions/embed": 6,
"eval/conjuncts": 3,
"eval/disjunctions": 3,
"eval/notify": 17,
"fulleval/054_issue312": 1,
"scalars/embed": 1,
Expand Down
20 changes: 14 additions & 6 deletions internal/core/adt/overlay.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,12 @@ func (ctx *overlayContext) cloneVertex(x *Vertex) *Vertex {

v._cc.src = v
v._cc.parentConjuncts = v

// The group of the root closeContext should point to the Conjuncts field
// of the Vertex. As we already allocated the group, we use that allocation,
// but "move" it to v.Conjuncts.
v.Conjuncts = *v._cc.group
v._cc.group = &v.Conjuncts

if a := x.Arcs; len(a) > 0 {
// TODO(perf): reuse buffer.
Expand Down Expand Up @@ -280,13 +285,16 @@ func (ctx *overlayContext) allocCC(cc *closeContext) *closeContext {
}

if o.parent != nil {
// validate invariants
ca := *cc.parent.group
if ca[cc.parentIndex].x != cc.group {
panic("group misaligned")
// validate invariants.
// TODO: the group can sometimes be empty. Investigate why and
// whether this is valid.
if ca := *cc.parent.group; len(ca) > 0 {
if ca[cc.parentIndex].x != cc.group {
panic("group misaligned")
}

(*o.parent.group)[cc.parentIndex].x = o.group
}

(*o.parent.group)[cc.parentIndex].x = o.group
}
}

Expand Down

0 comments on commit 96cff7c

Please sign in to comment.