From 95b6fc94440d56ea594bad2929decd35a198d5e5 Mon Sep 17 00:00:00 2001 From: N5N3 <2642243996@qq.com> Date: Sat, 18 May 2024 14:09:09 +0800 Subject: [PATCH 1/7] fix potential invalid memory access during typeintersect --- src/subtype.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/subtype.c b/src/subtype.c index 15bd56bdf4275..43900f7dc0c4f 100644 --- a/src/subtype.c +++ b/src/subtype.c @@ -2958,9 +2958,11 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind } } if (varval) { - *btemp->ub = jl_substitute_var_nothrow(iub, vb->var, varval); - if (*btemp->ub == NULL) + iub = jl_substitute_var_nothrow(iub, vb->var, varval, 2); + if (iub == NULL) res = jl_bottom_type; + else + *btemp->ub = iub; } else if (iub == (jl_value_t*)vb->var) { // TODO: this loses some constraints, such as in this test, where we replace T4<:S3 (e.g. T4==S3 since T4 only appears covariantly once) with T4<:Any From 53d65ad05b4c2d36c8c0af446cf302d3f77f6111 Mon Sep 17 00:00:00 2001 From: N5N3 <2642243996@qq.com> Date: Sat, 18 May 2024 14:15:05 +0800 Subject: [PATCH 2/7] add two level nothrow control for type instantiation This commit adds a two level control for nothrow path. `nothrow == 1` means instantiation should not throw but the result must be precise. `nothrow == 2` further remove the second requirement. With this change, then we can remove those Try/Catch blocks. --- src/jltypes.c | 54 ++++++++++++++++++++++---------------------- src/julia_internal.h | 2 +- src/subtype.c | 6 ++--- 3 files changed, 31 insertions(+), 31 deletions(-) diff --git a/src/jltypes.c b/src/jltypes.c index 420b88aeb7f92..46fe1779577af 100644 --- a/src/jltypes.c +++ b/src/jltypes.c @@ -1499,11 +1499,11 @@ jl_unionall_t *jl_rename_unionall(jl_unionall_t *u) return (jl_unionall_t*)t; } -jl_value_t *jl_substitute_var_nothrow(jl_value_t *t, jl_tvar_t *var, jl_value_t *val) +jl_value_t *jl_substitute_var_nothrow(jl_value_t *t, jl_tvar_t *var, jl_value_t *val, int nothrow) { if (val == (jl_value_t*)var) return t; - int nothrow = jl_is_typevar(val) ? 0 : 1; + nothrow = jl_is_typevar(val) ? 0 : nothrow; jl_typeenv_t env = { var, val, NULL }; return inst_type_w_(t, &env, NULL, 1, nothrow); } @@ -2401,7 +2401,8 @@ static jl_value_t *inst_tuple_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_ jl_value_t *elt = jl_svecref(tp, i); jl_value_t *pi = inst_type_w_(elt, env, stack, check, nothrow); if (pi == NULL) { - if (i == ntp-1 && jl_is_vararg(elt)) { + assert(nothrow); + if (nothrow == 1 || (i == ntp-1 && jl_is_vararg(elt))) { t = NULL; break; } @@ -2440,11 +2441,10 @@ static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t jl_value_t *var = NULL; jl_value_t *newbody = NULL; JL_GC_PUSH3(&lb, &var, &newbody); - JL_TRY { - lb = inst_type_w_(ua->var->lb, env, stack, check, 0); - } - JL_CATCH { - if (!nothrow) jl_rethrow(); + // set nothrow <= 1 to ensure lb's accuracy. + lb = inst_type_w_(ua->var->lb, env, stack, check, nothrow ? 1 : 0); + if (lb == NULL) { + assert(nothrow); t = NULL; } if (t != NULL) { @@ -2477,6 +2477,7 @@ static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t } else if (newbody != ua->body || var != (jl_value_t*)ua->var) { // if t's parameters are not bound in the environment, return it uncopied (#9378) + assert(jl_has_typevar(newbody, (jl_tvar_t *)var)); t = jl_new_struct(jl_unionall_type, var, newbody); } } @@ -2494,11 +2495,9 @@ static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t // fast path for `jl_rename_unionall`. t = jl_new_struct(jl_uniontype_type, a, b); } - else if (nothrow && a == NULL) { - t = b; - } - else if (nothrow && b == NULL) { - t = a; + else if (a == NULL || b == NULL) { + assert(nothrow); + t = nothrow == 1 ? NULL : a == NULL ? b : a; } else { assert(a != NULL && b != NULL); @@ -2516,15 +2515,17 @@ static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t JL_GC_PUSH2(&T, &N); if (v->T) { T = inst_type_w_(v->T, env, stack, check, nothrow); - if (T == NULL) - T = jl_bottom_type; - if (v->N) // This branch should never throw. + if (T == NULL) { + if (nothrow == 2) + T = jl_bottom_type; + else + t = NULL; + } + if (t && v->N) // This branch should never throw. N = inst_type_w_(v->N, env, stack, check, 0); } - if (T != v->T || N != v->N) { - // `Vararg` is special, we'd better handle inner error at Tuple level. + if (t && (T != v->T || N != v->N)) t = (jl_value_t*)jl_wrap_vararg(T, N, check, nothrow); - } JL_GC_POP(); return t; } @@ -2543,16 +2544,15 @@ static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t int bound = 0; for (i = 0; i < ntp; i++) { jl_value_t *elt = jl_svecref(tp, i); - JL_TRY { - jl_value_t *pi = inst_type_w_(elt, env, stack, check, 0); - iparams[i] = pi; - bound |= (pi != elt); - } - JL_CATCH { - if (!nothrow) jl_rethrow(); + // set nothrow <= 1 to ensure invariant parameter's accuracy. + jl_value_t *pi = inst_type_w_(elt, env, stack, check, nothrow ? 1 : 0); + if (pi == NULL) { + assert(nothrow); t = NULL; + break; } - if (t == NULL) break; + iparams[i] = pi; + bound |= (pi != elt); } // if t's parameters are not bound in the environment, return it uncopied (#9378) if (t != NULL && bound) diff --git a/src/julia_internal.h b/src/julia_internal.h index 396aadf40f8b7..62506cb6c43ae 100644 --- a/src/julia_internal.h +++ b/src/julia_internal.h @@ -757,7 +757,7 @@ JL_DLLEXPORT int jl_type_morespecific_no_subtype(jl_value_t *a, jl_value_t *b); jl_value_t *jl_instantiate_type_with(jl_value_t *t, jl_value_t **env, size_t n); JL_DLLEXPORT jl_value_t *jl_instantiate_type_in_env(jl_value_t *ty, jl_unionall_t *env, jl_value_t **vals); jl_value_t *jl_substitute_var(jl_value_t *t, jl_tvar_t *var, jl_value_t *val); -jl_value_t *jl_substitute_var_nothrow(jl_value_t *t, jl_tvar_t *var, jl_value_t *val); +jl_value_t *jl_substitute_var_nothrow(jl_value_t *t, jl_tvar_t *var, jl_value_t *val, int nothrow); jl_unionall_t *jl_rename_unionall(jl_unionall_t *u); JL_DLLEXPORT jl_value_t *jl_unwrap_unionall(jl_value_t *v JL_PROPAGATES_ROOT) JL_NOTSAFEPOINT; JL_DLLEXPORT jl_value_t *jl_rewrap_unionall(jl_value_t *t, jl_value_t *u); diff --git a/src/subtype.c b/src/subtype.c index 43900f7dc0c4f..8e361f44914df 100644 --- a/src/subtype.c +++ b/src/subtype.c @@ -2784,7 +2784,7 @@ static jl_value_t *omit_bad_union(jl_value_t *u, jl_tvar_t *t) res = jl_bottom_type; } else if (obviously_egal(var->lb, ub)) { - res = jl_substitute_var_nothrow(body, var, ub); + res = jl_substitute_var_nothrow(body, var, ub, 2); if (res == NULL) res = jl_bottom_type; } @@ -3093,12 +3093,12 @@ static jl_value_t *finish_unionall(jl_value_t *res JL_MAYBE_UNROOTED, jl_varbind if (varval) { // you can construct `T{x} where x` even if T's parameter is actually // limited. in that case we might get an invalid instantiation here. - res = jl_substitute_var_nothrow(res, vb->var, varval); + res = jl_substitute_var_nothrow(res, vb->var, varval, 2); // simplify chains of UnionAlls where bounds become equal while (res != NULL && jl_is_unionall(res) && obviously_egal(((jl_unionall_t*)res)->var->lb, ((jl_unionall_t*)res)->var->ub)) { jl_unionall_t * ures = (jl_unionall_t *)res; - res = jl_substitute_var_nothrow(ures->body, ures->var, ures->var->lb); + res = jl_substitute_var_nothrow(ures->body, ures->var, ures->var->lb, 2); } if (res == NULL) res = jl_bottom_type; From 28da0b72d0dba6ec0e40e7fe3c64d605c647e886 Mon Sep 17 00:00:00 2001 From: N5N3 <2642243996@qq.com> Date: Sat, 18 May 2024 14:19:37 +0800 Subject: [PATCH 3/7] propagate nothrow flag into `check_datatype_parameters` --- src/jltypes.c | 28 +++++++++++++++++++--------- test/subtype.jl | 6 ++++++ 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/jltypes.c b/src/jltypes.c index 46fe1779577af..169eb1cf4155d 100644 --- a/src/jltypes.c +++ b/src/jltypes.c @@ -1725,7 +1725,7 @@ void jl_precompute_memoized_dt(jl_datatype_t *dt, int cacheable) dt->hash = typekey_hash(dt->name, jl_svec_data(dt->parameters), l, cacheable); } -static void check_datatype_parameters(jl_typename_t *tn, jl_value_t **params, size_t np) +static int check_datatype_parameters(jl_typename_t *tn, jl_value_t **params, size_t np, int nothrow) { jl_value_t *wrapper = tn->wrapper; jl_value_t **bounds; @@ -1743,6 +1743,10 @@ static void check_datatype_parameters(jl_typename_t *tn, jl_value_t **params, si assert(jl_is_unionall(wrapper)); jl_tvar_t *tv = ((jl_unionall_t*)wrapper)->var; if (!within_typevar(params[i], bounds[2*i], bounds[2*i+1])) { + if (nothrow) { + JL_GC_POP(); + return 1; + } if (tv->lb != bounds[2*i] || tv->ub != bounds[2*i+1]) // pass a new version of `tv` containing the instantiated bounds tv = jl_new_typevar(tv->name, bounds[2*i], bounds[2*i+1]); @@ -1752,12 +1756,23 @@ static void check_datatype_parameters(jl_typename_t *tn, jl_value_t **params, si int j; for (j = 2*i + 2; j < 2*np; j++) { jl_value_t *bj = bounds[j]; - if (bj != (jl_value_t*)jl_any_type && bj != jl_bottom_type) - bounds[j] = jl_substitute_var(bj, tv, params[i]); + if (bj != (jl_value_t*)jl_any_type && bj != jl_bottom_type) { + int isub = j & 1; + // use different nothrow level for lb and ub substitution. + // TODO: should normal path of type instantiation follows this rule? + jl_value_t *nb = jl_substitute_var_nothrow(bj, tv, params[i], nothrow ? (isub ? 2 : 1) : 0 ); + if (nb == NULL) { + assert(nothrow); + JL_GC_POP(); + return 1; + } + bounds[j] = nb; + } } wrapper = ((jl_unionall_t*)wrapper)->body; } JL_GC_POP(); + return 0; } static jl_value_t *extract_wrapper(jl_value_t *t JL_PROPAGATES_ROOT) JL_NOTSAFEPOINT JL_GLOBALLY_ROOTED @@ -2004,13 +2019,8 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value // for whether this is even valid if (check && !istuple) { assert(ntp > 0); - JL_TRY { - check_datatype_parameters(tn, iparams, ntp); - } - JL_CATCH { - if (!nothrow) jl_rethrow(); + if (check_datatype_parameters(tn, iparams, ntp, nothrow)) return NULL; - } } else if (ntp == 0 && jl_emptytuple_type != NULL) { // empty tuple type case diff --git a/test/subtype.jl b/test/subtype.jl index 04171e37b34fb..dbfb63757e269 100644 --- a/test/subtype.jl +++ b/test/subtype.jl @@ -2617,10 +2617,16 @@ end #issue 54356 abstract type A54356{T<:Real} end struct B54356{T} <: A54356{T} end +struct C54356{S,T<:Union{S,Complex{S}}} end let S = Tuple{Val, Val{T}} where {T}, R = Tuple{Val{Val{T}}, Val{T}} where {T} # general parameters check @testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, Complex{B}}}, S{1}, R{1}) @testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, B54356{B}}}, S{1}, R{1}) + # `check_datatype_parameters` should ignore bad `Union` elements in constraint's ub + T = Tuple{Val{Union{Val{Nothing}, Val{C54356{V,V}}}}, Val{Nothing}} where {Nothing<:V<:Nothing} + @test T <: S{Nothing} + @test T <: Tuple{Val{A}, A} where {B, C, A<:Union{Val{B}, Val{C54356{B,C}}}} + @test T <: typeintersect(Tuple{Val{A}, A} where {B, C, A<:Union{Val{B}, Val{C54356{B,C}}}}, S{Nothing}) # extra check for Vararg @testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, NTuple{B,Any}}}, S{-1}, R{-1}) @testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, Tuple{Any,Vararg{Any,B}}}}, S{-1}, R{-1}) From 1bac8dd130339706a9fc7888c9ded93e36958617 Mon Sep 17 00:00:00 2001 From: N5N3 <2642243996@qq.com> Date: Sat, 18 May 2024 14:22:59 +0800 Subject: [PATCH 4/7] fix some instantiation error during `finish_unionall` --- src/jltypes.c | 15 +++++++++++++-- test/subtype.jl | 15 +++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/jltypes.c b/src/jltypes.c index 169eb1cf4155d..66152dec464bb 100644 --- a/src/jltypes.c +++ b/src/jltypes.c @@ -2500,6 +2500,13 @@ static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t jl_value_t *b = NULL; JL_GC_PUSH2(&a, &b); b = inst_type_w_(u->b, env, stack, check, nothrow); + if (nothrow) { + // ensure jl_type_union nothrow. + if (a && !(jl_is_typevar(a) || jl_is_type(a))) + a = NULL; + if (b && !(jl_is_typevar(b) || jl_is_type(b))) + b = NULL; + } if (a != u->a || b != u->b) { if (!check) { // fast path for `jl_rename_unionall`. @@ -2531,8 +2538,12 @@ static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t else t = NULL; } - if (t && v->N) // This branch should never throw. - N = inst_type_w_(v->N, env, stack, check, 0); + if (t && v->N) { + // set nothrow <= 1 to ensure invariant parameter's accuracy. + N = inst_type_w_(v->N, env, stack, check, nothrow ? 1 : 0); + if (N == NULL) + t = NULL; + } } if (t && (T != v->T || N != v->N)) t = (jl_value_t*)jl_wrap_vararg(T, N, check, nothrow); diff --git a/test/subtype.jl b/test/subtype.jl index dbfb63757e269..c65521d44ac5a 100644 --- a/test/subtype.jl +++ b/test/subtype.jl @@ -2618,10 +2618,20 @@ end abstract type A54356{T<:Real} end struct B54356{T} <: A54356{T} end struct C54356{S,T<:Union{S,Complex{S}}} end -let S = Tuple{Val, Val{T}} where {T}, R = Tuple{Val{Val{T}}, Val{T}} where {T} - # general parameters check +struct D54356{S<:Real,T} end +let S = Tuple{Val, Val{T}} where {T}, R = Tuple{Val{Val{T}}, Val{T}} where {T}, + SS = Tuple{Val, Val{T}, Val{T}} where {T}, RR = Tuple{Val{Val{T}}, Val{T}, Val{T}} where {T} + # parameters check for self @testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, Complex{B}}}, S{1}, R{1}) + # parameters check for supertype (B54356 -> A54356) @testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, B54356{B}}}, S{1}, R{1}) + # enure unused TypeVar skips the `UnionAll` wrapping + @testintersect(Tuple{Val{A}, A} where {B, A<:(Union{Val{B}, D54356{B,C}} where {C})}, S{1}, R{1}) + # invariant parameter should not get narrowed + @testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, Val{Union{Int,Complex{B}}}}}, S{1}, R{1}) + # bit value could not be `Union` element + @testintersect(Tuple{Val{A}, A, Val{B}} where {B, A<:Union{B, Val{B}}}, SS{1}, RR{1}) + @testintersect(Tuple{Val{A}, A, Val{B}} where {B, A<:Union{B, Complex{B}}}, SS{1}, Union{}) # `check_datatype_parameters` should ignore bad `Union` elements in constraint's ub T = Tuple{Val{Union{Val{Nothing}, Val{C54356{V,V}}}}, Val{Nothing}} where {Nothing<:V<:Nothing} @test T <: S{Nothing} @@ -2630,6 +2640,7 @@ let S = Tuple{Val, Val{T}} where {T}, R = Tuple{Val{Val{T}}, Val{T}} where {T} # extra check for Vararg @testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, NTuple{B,Any}}}, S{-1}, R{-1}) @testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, Tuple{Any,Vararg{Any,B}}}}, S{-1}, R{-1}) + @testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, Tuple{Vararg{Int,Union{Int,Complex{B}}}}}}, S{1}, R{1}) # extra check for NamedTuple @testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, NamedTuple{B,Tuple{Int}}}}, S{1}, R{1}) @testintersect(Tuple{Val{A}, A} where {B, A<:Union{Val{B}, NamedTuple{B,Tuple{Int}}}}, S{(1,)}, R{(1,)}) From 202bae91a419ce0f595601695ecfe40fffec09cb Mon Sep 17 00:00:00 2001 From: N5N3 <2642243996@qq.com> Date: Sat, 18 May 2024 18:06:19 +0800 Subject: [PATCH 5/7] fix assert error --- src/jltypes.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/jltypes.c b/src/jltypes.c index 66152dec464bb..e63b7b0091bcd 100644 --- a/src/jltypes.c +++ b/src/jltypes.c @@ -2478,16 +2478,13 @@ static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t if (newbody == NULL) { t = NULL; } - else if (newbody == (jl_value_t*)jl_emptytuple_type) { - // NTuple{0} => Tuple{} can make a typevar disappear - t = (jl_value_t*)jl_emptytuple_type; - } - else if (nothrow && !jl_has_typevar(newbody, (jl_tvar_t *)var)) { + else if (!jl_has_typevar(newbody, (jl_tvar_t *)var)) { + // inner instantiation might make a typevar disappear, e.g. + // NTuple{0,T} => Tuple{} t = newbody; } else if (newbody != ua->body || var != (jl_value_t*)ua->var) { // if t's parameters are not bound in the environment, return it uncopied (#9378) - assert(jl_has_typevar(newbody, (jl_tvar_t *)var)); t = jl_new_struct(jl_unionall_type, var, newbody); } } From f35bb32a76609f2c97d34f8119263e68e2fd809d Mon Sep 17 00:00:00 2001 From: N5N3 <2642243996@qq.com> Date: Sun, 19 May 2024 07:41:25 +0800 Subject: [PATCH 6/7] comment the usage of nothrow flag --- src/jltypes.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/jltypes.c b/src/jltypes.c index e63b7b0091bcd..700b0c4685ecf 100644 --- a/src/jltypes.c +++ b/src/jltypes.c @@ -1759,7 +1759,10 @@ static int check_datatype_parameters(jl_typename_t *tn, jl_value_t **params, siz if (bj != (jl_value_t*)jl_any_type && bj != jl_bottom_type) { int isub = j & 1; // use different nothrow level for lb and ub substitution. - // TODO: should normal path of type instantiation follows this rule? + // TODO: This assuming the top instantiation could only start with + // `nothrow == 2` or `nothrow == 0`. If `nothrow` is initially set to 1 + // then we might miss some inner error, perhaps the normal path should + // also follow this rule? jl_value_t *nb = jl_substitute_var_nothrow(bj, tv, params[i], nothrow ? (isub ? 2 : 1) : 0 ); if (nb == NULL) { assert(nothrow); @@ -2431,6 +2434,10 @@ static jl_value_t *inst_tuple_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_ return t; } +// `nothrow` means that when type checking fails, the type instantiation should +// return `NULL` instead of immediately throwing an error. If `nothrow` == 2 then +// we further assume that the imprecise instantiation for non invariant parameters +// is acceptable, and inner error (`NULL`) would be ignored. static jl_value_t *inst_type_w_(jl_value_t *t, jl_typeenv_t *env, jl_typestack_t *stack, int check, int nothrow) { size_t i; From aa04c4aba25f0fe5387973afe57bc156708f3207 Mon Sep 17 00:00:00 2001 From: N5N3 <2642243996@qq.com> Date: Sun, 19 May 2024 10:24:54 +0800 Subject: [PATCH 7/7] whitespace --- src/jltypes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/jltypes.c b/src/jltypes.c index 700b0c4685ecf..79cb787ab042f 100644 --- a/src/jltypes.c +++ b/src/jltypes.c @@ -1761,7 +1761,7 @@ static int check_datatype_parameters(jl_typename_t *tn, jl_value_t **params, siz // use different nothrow level for lb and ub substitution. // TODO: This assuming the top instantiation could only start with // `nothrow == 2` or `nothrow == 0`. If `nothrow` is initially set to 1 - // then we might miss some inner error, perhaps the normal path should + // then we might miss some inner error, perhaps the normal path should // also follow this rule? jl_value_t *nb = jl_substitute_var_nothrow(bj, tv, params[i], nothrow ? (isub ? 2 : 1) : 0 ); if (nb == NULL) {