From 96c80a2dd1fcf3060d8dfae0a7ece76825395ffc Mon Sep 17 00:00:00 2001 From: Simon Hausmann Date: Sat, 14 Jan 2023 21:20:14 +0100 Subject: [PATCH] Fix crash when using an int model in a repeater with a negative value (#2063) Make sure that we return an unsigned for row_count() in C++ and Rust by ensuring an unsigned int model at creation time. For the interpreter this "worked" by chance as casting a negative floating number to usize automatically caps at zero, and all values are stored as f64. For safety this patch applies the same fix though, to be on the safe side. --- api/cpp/include/slint.h | 6 +-- internal/compiler/generator/cpp.rs | 4 +- internal/compiler/generator/rust.rs | 2 +- internal/interpreter/value_model.rs | 2 +- tests/cases/models/negative_intmodel.slint | 63 ++++++++++++++++++++++ 5 files changed, 70 insertions(+), 7 deletions(-) create mode 100644 tests/cases/models/negative_intmodel.slint diff --git a/api/cpp/include/slint.h b/api/cpp/include/slint.h index be9dceb50..69815db33 100644 --- a/api/cpp/include/slint.h +++ b/api/cpp/include/slint.h @@ -768,12 +768,12 @@ public: }; /// Model to be used when we just want to repeat without data. -struct IntModel : Model +struct UIntModel : Model { /// Constructs a new IntModel with \a d rows. - IntModel(int d) : data(d) { } + UIntModel(uint32_t d) : data(d) { } /// \private - int data; + uint32_t data; /// \copydoc Model::row_count int row_count() const override { return data; } std::optional row_data(int value) const override diff --git a/internal/compiler/generator/cpp.rs b/internal/compiler/generator/cpp.rs index 81b828352..52d624dd1 100644 --- a/internal/compiler/generator/cpp.rs +++ b/internal/compiler/generator/cpp.rs @@ -1466,7 +1466,7 @@ fn generate_sub_component( if repeated.model.ty(&ctx) == Type::Bool { // bool converts to int // FIXME: don't do a heap allocation here - model = format!("std::make_shared({})", model) + model = format!("std::make_shared({})", model) } // FIXME: optimize if repeated.model.is_constant() @@ -2247,7 +2247,7 @@ fn compile_expression(expr: &llr::Expression, ctx: &EvaluationContext) -> String format!("slint::SharedString::from_number({})", f) } (Type::Float32, Type::Model) | (Type::Int32, Type::Model) => { - format!("std::make_shared({})", f) + format!("std::make_shared(std::max(0, {}))", f) } (Type::Array(_), Type::Model) => f, (Type::Float32, Type::Color) => { diff --git a/internal/compiler/generator/rust.rs b/internal/compiler/generator/rust.rs index 5ff867a93..81b26bc28 100644 --- a/internal/compiler/generator/rust.rs +++ b/internal/compiler/generator/rust.rs @@ -1785,7 +1785,7 @@ fn compile_expression(expr: &Expression, ctx: &EvaluationContext) -> TokenStream )) } (Type::Float32, Type::Model) | (Type::Int32, Type::Model) => { - quote!(slint::private_unstable_api::re_exports::ModelRc::new(#f as usize)) + quote!(slint::private_unstable_api::re_exports::ModelRc::new(#f.max(Default::default()) as usize)) } (Type::Float32, Type::Color) => { quote!(slint::private_unstable_api::re_exports::Color::from_argb_encoded(#f as u32)) diff --git a/internal/interpreter/value_model.rs b/internal/interpreter/value_model.rs index 35969693e..c650fbf5a 100644 --- a/internal/interpreter/value_model.rs +++ b/internal/interpreter/value_model.rs @@ -47,7 +47,7 @@ impl Model for ValueModel { 0 } } - Value::Number(x) => *x as usize, + Value::Number(x) => x.max(Default::default()) as usize, Value::Void => 0, Value::Model(model_ptr) => model_ptr.row_count(), x => panic!("Invalid model {:?}", x), diff --git a/tests/cases/models/negative_intmodel.slint b/tests/cases/models/negative_intmodel.slint new file mode 100644 index 000000000..0f26f7c1b --- /dev/null +++ b/tests/cases/models/negative_intmodel.slint @@ -0,0 +1,63 @@ +// Copyright © SixtyFPS GmbH +// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-commercial + + + +TestCase := Rectangle { + width: 100phx; + height: 100phx; + + property creation-count: 0; + + property test-height: preferred-height; + + property repeater-count: -10; + + VerticalLayout { + for _ in repeater-count: Rectangle { + preferred-height: 10px; + init => { + creation-count += 1; + } + } + } + +} + + +/* +```cpp +auto handle = TestCase::create(); +const TestCase &instance = *handle; + +assert_eq(instance.get_creation_count(), 0); +assert_eq(instance.get_test_height(), 0.); +assert_eq(instance.get_creation_count(), 0); +instance.set_repeater_count(2); +assert_eq(instance.get_test_height(), 20.); +assert_eq(instance.get_creation_count(), 2); +``` + + +```rust +let instance = TestCase::new(); + +assert_eq!(instance.get_creation_count(), 0); +assert_eq!(instance.get_test_height(), 0.); +assert_eq!(instance.get_creation_count(), 0); +instance.set_repeater_count(2); +assert_eq!(instance.get_test_height(), 20.); +assert_eq!(instance.get_creation_count(), 2); +``` + +```js +var instance = new slint.TestCase(); + +assert.equal(instance.creation_count, 0); +assert.equal(instance.test_height, 0.); +assert.equal(instance.creation_count, 0); +instance.repeater_count = 2; +assert.equal(instance.test_height, 20.); +assert.equal(instance.creation_count, 2); +``` +*/