From cf60895090010ec4efcc23238d79f22ee54e9db5 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Tue, 26 Sep 2023 18:55:12 -0700 Subject: [PATCH] Prevent use of uninitialized scalar Parameters in JIT code (#7847, partial) (#7853) * Prevent use of uninitialized scalar Parameters in JIT code (#7847, partial) * Fix broken tests * Update Parameter.h * Update func_clone.cpp * Fix Generators too * Fixes * Update InferArguments.cpp * Fixes * pacify clang-tidy * fixes --- .../src/halide/halide_/PyParameter.cpp | 5 -- src/Deserialization.cpp | 16 +++- src/Generator.h | 3 +- src/InferArguments.cpp | 5 +- src/Parameter.cpp | 90 +++++++++++++------ src/Parameter.h | 71 +++++++++------ src/Pipeline.cpp | 2 +- src/Serialization.cpp | 10 ++- src/halide_ir.fbs | 2 +- test/correctness/func_clone.cpp | 1 + test/correctness/specialize.cpp | 4 + test/error/CMakeLists.txt | 2 + test/error/uninitialized_param.cpp | 22 +++++ test/error/uninitialized_param_2.cpp | 46 ++++++++++ 14 files changed, 213 insertions(+), 66 deletions(-) create mode 100644 test/error/uninitialized_param.cpp create mode 100644 test/error/uninitialized_param_2.cpp diff --git a/python_bindings/src/halide/halide_/PyParameter.cpp b/python_bindings/src/halide/halide_/PyParameter.cpp index 87da6610a691..8464ed387d23 100644 --- a/python_bindings/src/halide/halide_/PyParameter.cpp +++ b/python_bindings/src/halide/halide_/PyParameter.cpp @@ -30,11 +30,6 @@ void define_parameter(py::module &m) { .def(py::init(), py::arg("p")) .def(py::init()) .def(py::init()) - .def(py::init &, int, const std::vector &, - MemoryType>()) - .def(py::init()) .def("_to_argument", [](const Parameter &p) -> Argument { return Argument(p.name(), p.is_buffer() ? Argument::InputBuffer : Argument::InputScalar, diff --git a/src/Deserialization.cpp b/src/Deserialization.cpp index c56badf7b887..b882f8b0b157 100644 --- a/src/Deserialization.cpp +++ b/src/Deserialization.cpp @@ -1159,14 +1159,24 @@ Parameter Deserializer::deserialize_parameter(const Serialize::Parameter *parame deserialize_vector(parameter->buffer_constraints(), &Deserializer::deserialize_buffer_constraint); const auto memory_type = deserialize_memory_type(parameter->memory_type()); - return Parameter(type, is_buffer, dimensions, name, Buffer<>(), host_alignment, buffer_constraints, memory_type); + return Parameter(type, dimensions, name, Buffer<>(), host_alignment, buffer_constraints, memory_type); } else { - const uint64_t data = parameter->data(); + static_assert(FLATBUFFERS_USE_STD_OPTIONAL); + const auto make_optional_halide_scalar_value_t = [](const std::optional &v) -> std::optional { + if (v.has_value()) { + halide_scalar_value_t scalar_data; + scalar_data.u.u64 = v.value(); + return std::optional(scalar_data); + } else { + return std::nullopt; + } + }; + const std::optional scalar_data = make_optional_halide_scalar_value_t(parameter->scalar_data()); const auto scalar_default = deserialize_expr(parameter->scalar_default_type(), parameter->scalar_default()); const auto scalar_min = deserialize_expr(parameter->scalar_min_type(), parameter->scalar_min()); const auto scalar_max = deserialize_expr(parameter->scalar_max_type(), parameter->scalar_max()); const auto scalar_estimate = deserialize_expr(parameter->scalar_estimate_type(), parameter->scalar_estimate()); - return Parameter(type, is_buffer, dimensions, name, data, scalar_default, scalar_min, scalar_max, scalar_estimate); + return Parameter(type, dimensions, name, scalar_data, scalar_default, scalar_min, scalar_max, scalar_estimate); } } diff --git a/src/Generator.h b/src/Generator.h index d0614b24efb9..9bc335b52ed7 100644 --- a/src/Generator.h +++ b/src/Generator.h @@ -2001,7 +2001,8 @@ class GeneratorInput_Scalar : public GeneratorInputImpl { void set_def_min_max() override { for (Parameter &p : this->parameters_) { - p.set_scalar(def_); + // No: we want to leave the Parameter unset here. + // p.set_scalar(def_); p.set_default_value(def_expr_); } } diff --git a/src/InferArguments.cpp b/src/InferArguments.cpp index d2f55b1fa781..020d4184642d 100644 --- a/src/InferArguments.cpp +++ b/src/InferArguments.cpp @@ -5,6 +5,7 @@ #include "ExternFuncArgument.h" #include "Function.h" +#include "IROperator.h" #include "IRVisitor.h" #include "InferArguments.h" @@ -197,7 +198,9 @@ class InferArguments : public IRGraphVisitor { ArgumentEstimates argument_estimates = p.get_argument_estimates(); if (!p.is_buffer()) { - argument_estimates.scalar_def = p.scalar_expr(); + // We don't want to crater here if a scalar param isn't set; + // instead, default to a zero of the right type, like we used to. + argument_estimates.scalar_def = p.has_scalar_value() ? p.scalar_expr() : make_zero(p.type()); argument_estimates.scalar_min = p.min_value(); argument_estimates.scalar_max = p.max_value(); argument_estimates.scalar_estimate = p.estimate(); diff --git a/src/Parameter.cpp b/src/Parameter.cpp index 1155d5468f30..d9616b5bebf8 100644 --- a/src/Parameter.cpp +++ b/src/Parameter.cpp @@ -15,7 +15,7 @@ struct ParameterContents { const int dimensions; const std::string name; Buffer<> buffer; - uint64_t data = 0; + std::optional scalar_data; int host_alignment; std::vector buffer_constraints; Expr scalar_default, scalar_min, scalar_max, scalar_estimate; @@ -82,21 +82,21 @@ Parameter::Parameter(const Type &t, bool is_buffer, int d, const std::string &na internal_assert(is_buffer || d == 0) << "Scalar parameters should be zero-dimensional"; } -Parameter::Parameter(const Type &t, bool is_buffer, int dimensions, const std::string &name, +Parameter::Parameter(const Type &t, int dimensions, const std::string &name, const Buffer &buffer, int host_alignment, const std::vector &buffer_constraints, MemoryType memory_type) - : contents(new Internal::ParameterContents(t, is_buffer, dimensions, name)) { + : contents(new Internal::ParameterContents(t, /*is_buffer*/ true, dimensions, name)) { contents->buffer = buffer; contents->host_alignment = host_alignment; contents->buffer_constraints = buffer_constraints; contents->memory_type = memory_type; } -Parameter::Parameter(const Type &t, bool is_buffer, int dimensions, const std::string &name, - uint64_t data, const Expr &scalar_default, const Expr &scalar_min, +Parameter::Parameter(const Type &t, int dimensions, const std::string &name, + const std::optional &scalar_data, const Expr &scalar_default, const Expr &scalar_min, const Expr &scalar_max, const Expr &scalar_estimate) - : contents(new Internal::ParameterContents(t, is_buffer, dimensions, name)) { - contents->data = data; + : contents(new Internal::ParameterContents(t, /*is_buffer*/ false, dimensions, name)) { + contents->scalar_data = scalar_data; contents->scalar_default = scalar_default; contents->scalar_min = scalar_min; contents->scalar_max = scalar_max; @@ -123,51 +123,55 @@ bool Parameter::is_buffer() const { return contents->is_buffer; } +bool Parameter::has_scalar_value() const { + return defined() && !contents->is_buffer && contents->scalar_data.has_value(); +} + Expr Parameter::scalar_expr() const { - check_is_scalar(); + const auto sv = scalar_data_checked(); const Type t = type(); if (t.is_float()) { switch (t.bits()) { case 16: if (t.is_bfloat()) { - return Expr(scalar()); + return Expr(bfloat16_t::make_from_bits(sv.u.u16)); } else { - return Expr(scalar()); + return Expr(float16_t::make_from_bits(sv.u.u16)); } case 32: - return Expr(scalar()); + return Expr(sv.u.f32); case 64: - return Expr(scalar()); + return Expr(sv.u.f64); } } else if (t.is_int()) { switch (t.bits()) { case 8: - return Expr(scalar()); + return Expr(sv.u.i8); case 16: - return Expr(scalar()); + return Expr(sv.u.i16); case 32: - return Expr(scalar()); + return Expr(sv.u.i32); case 64: - return Expr(scalar()); + return Expr(sv.u.i64); } } else if (t.is_uint()) { switch (t.bits()) { case 1: - return Internal::make_bool(scalar()); + return Internal::make_bool(sv.u.b); case 8: - return Expr(scalar()); + return Expr(sv.u.u8); case 16: - return Expr(scalar()); + return Expr(sv.u.u16); case 32: - return Expr(scalar()); + return Expr(sv.u.u32); case 64: - return Expr(scalar()); + return Expr(sv.u.u64); } } else if (t.is_handle()) { // handles are always uint64 internally. switch (t.bits()) { case 64: - return Expr(scalar()); + return Expr(sv.u.u64); } } internal_error << "Unsupported type " << t << " in scalar_expr\n"; @@ -198,14 +202,48 @@ void Parameter::set_buffer(const Buffer<> &b) { contents->buffer = b; } -void *Parameter::scalar_address() const { +const void *Parameter::read_only_scalar_address() const { check_is_scalar(); - return &contents->data; + // Use explicit if here (rather than user_assert) so that we don't + // have to disable bugprone-unchecked-optional-access in clang-tidy, + // which is a useful check. + const auto &sv = contents->scalar_data; + if (sv.has_value()) { + return std::addressof(sv.value()); + } else { + user_error << "Parameter " << name() << " does not have a valid scalar value.\n"; + return nullptr; + } +} + +std::optional Parameter::scalar_data() const { + return defined() ? contents->scalar_data : std::nullopt; } -uint64_t Parameter::scalar_raw_value() const { +halide_scalar_value_t Parameter::scalar_data_checked() const { check_is_scalar(); - return contents->data; + // Use explicit if here (rather than user_assert) so that we don't + // have to disable bugprone-unchecked-optional-access in clang-tidy, + // which is a useful check. + halide_scalar_value_t result; + const auto &sv = contents->scalar_data; + if (sv.has_value()) { + result = sv.value(); + } else { + user_error << "Parameter " << name() << " does not have a valid scalar value.\n"; + result.u.u64 = 0; // silence "possibly uninitialized" compiler warning + } + return result; +} + +halide_scalar_value_t Parameter::scalar_data_checked(const Type &val_type) const { + check_type(val_type); + return scalar_data_checked(); +} + +void Parameter::set_scalar(const Type &val_type, halide_scalar_value_t val) { + check_type(val_type); + contents->scalar_data = std::optional(val); } /** Tests if this handle is the same as another handle */ diff --git a/src/Parameter.h b/src/Parameter.h index 712380c2576e..09840683980c 100644 --- a/src/Parameter.h +++ b/src/Parameter.h @@ -4,6 +4,7 @@ /** \file * Defines the internal representation of parameters to halide piplines */ +#include #include #include "Buffer.h" @@ -25,7 +26,9 @@ struct BufferConstraint { }; namespace Internal { + #ifdef WITH_SERIALIZATION +class Deserializer; class Serializer; #endif struct ParameterContents; @@ -45,21 +48,41 @@ class Parameter { Internal::IntrusivePtr contents; #ifdef WITH_SERIALIZATION - friend class Internal::Serializer; //< for scalar_raw_value() + friend class Internal::Deserializer; //< for scalar_data() + friend class Internal::Serializer; //< for scalar_data() #endif - friend class Pipeline; //< for scalar_address() + friend class Pipeline; //< for read_only_scalar_address() /** Get the raw currently-bound buffer. null if unbound */ const halide_buffer_t *raw_buffer() const; /** Get the pointer to the current value of the scalar * parameter. For a given parameter, this address will never - * change. Only relevant when jitting. */ - void *scalar_address() const; + * change. Note that this can only be used to *read* from -- it must + * not be written to, so don't cast away the constness. Only relevant when jitting. */ + const void *read_only_scalar_address() const; + + /** If the Parameter is a scalar, and the scalar data is valid, return + * the scalar data. Otherwise, return nullopt. */ + std::optional scalar_data() const; + + /** If the Parameter is a scalar and has a valid scalar value, return it. + * Otherwise, assert-fail. */ + halide_scalar_value_t scalar_data_checked() const; + + /** If the Parameter is a scalar *of the given type* and has a valid scalar value, return it. + * Otherwise, assert-fail. */ + halide_scalar_value_t scalar_data_checked(const Type &val_type) const; - /** Get the raw data of the current value of the scalar - * parameter. Only relevant when serializing. */ - uint64_t scalar_raw_value() const; + /** Construct a new buffer parameter via deserialization. */ + Parameter(const Type &t, int dimensions, const std::string &name, + const Buffer &buffer, int host_alignment, const std::vector &buffer_constraints, + MemoryType memory_type); + + /** Construct a new scalar parameter via deserialization. */ + Parameter(const Type &t, int dimensions, const std::string &name, + const std::optional &scalar_data, const Expr &scalar_default, const Expr &scalar_min, + const Expr &scalar_max, const Expr &scalar_estimate); public: /** Construct a new undefined handle */ @@ -81,15 +104,6 @@ class Parameter { * explicitly specified (as opposed to autogenerated). */ Parameter(const Type &t, bool is_buffer, int dimensions, const std::string &name); - /** Construct a new parameter via deserialization. */ - Parameter(const Type &t, bool is_buffer, int dimensions, const std::string &name, - const Buffer &buffer, int host_alignment, const std::vector &buffer_constraints, - MemoryType memory_type); - - Parameter(const Type &t, bool is_buffer, int dimensions, const std::string &name, - uint64_t data, const Expr &scalar_default, const Expr &scalar_min, - const Expr &scalar_max, const Expr &scalar_estimate); - Parameter(const Parameter &) = default; Parameter &operator=(const Parameter &) = default; Parameter(Parameter &&) = default; @@ -111,28 +125,33 @@ class Parameter { * bound value. Only relevant when jitting */ template HALIDE_NO_USER_CODE_INLINE T scalar() const { - check_type(type_of()); - return *((const T *)(scalar_address())); + static_assert(sizeof(T) <= sizeof(halide_scalar_value_t)); + const auto sv = scalar_data_checked(type_of()); + T t; + memcpy(&t, &sv.u.u64, sizeof(t)); + return t; } - /** This returns the current value of scalar() - * as an Expr. */ + /** This returns the current value of scalar() as an Expr. + * If the Parameter is not scalar, or its scalar data is not valid, this will assert-fail. */ Expr scalar_expr() const; + /** This returns true if scalar_expr() would return a valid Expr, + * false if not. */ + bool has_scalar_value() const; + /** If the parameter is a scalar parameter, set its current * value. Only relevant when jitting */ template HALIDE_NO_USER_CODE_INLINE void set_scalar(T val) { - check_type(type_of()); - *((T *)(scalar_address())) = val; + halide_scalar_value_t sv; + memcpy(&sv.u.u64, &val, sizeof(val)); + set_scalar(type_of(), sv); } /** If the parameter is a scalar parameter, set its current * value. Only relevant when jitting */ - HALIDE_NO_USER_CODE_INLINE void set_scalar(const Type &val_type, halide_scalar_value_t val) { - check_type(val_type); - memcpy(scalar_address(), &val, val_type.bytes()); - } + void set_scalar(const Type &val_type, halide_scalar_value_t val); /** If the parameter is a buffer parameter, get its currently * bound buffer. Only relevant when jitting */ diff --git a/src/Pipeline.cpp b/src/Pipeline.cpp index d51438e275fe..631033404137 100644 --- a/src/Pipeline.cpp +++ b/src/Pipeline.cpp @@ -843,7 +843,7 @@ void Pipeline::prepare_jit_call_arguments(RealizationArg &outputs, const Target } debug(2) << "JIT input ImageParam argument "; } else { - args_result.store[arg_index++] = p.scalar_address(); + args_result.store[arg_index++] = p.read_only_scalar_address(); debug(2) << "JIT input scalar argument "; } } diff --git a/src/Serialization.cpp b/src/Serialization.cpp index cc9daa9fe325..d9afcb5e083b 100644 --- a/src/Serialization.cpp +++ b/src/Serialization.cpp @@ -1298,13 +1298,19 @@ Offset Serializer::serialize_parameter(FlatBufferBuilder & return Serialize::CreateParameter(builder, defined, is_buffer, type_serialized, dimensions, name_serialized, host_alignment, builder.CreateVector(buffer_constraints_serialized), memory_type_serialized); } else { - const uint64_t data = parameter.scalar_raw_value(); + static_assert(FLATBUFFERS_USE_STD_OPTIONAL); + const auto make_optional_u64 = [](const std::optional &v) -> std::optional { + return v.has_value() ? + std::optional(v.value().u.u64) : + std::nullopt; + }; + const auto scalar_data = make_optional_u64(parameter.scalar_data()); const auto scalar_default_serialized = serialize_expr(builder, parameter.default_value()); const auto scalar_min_serialized = serialize_expr(builder, parameter.min_value()); const auto scalar_max_serialized = serialize_expr(builder, parameter.max_value()); const auto scalar_estimate_serialized = serialize_expr(builder, parameter.estimate()); return Serialize::CreateParameter(builder, defined, is_buffer, type_serialized, - dimensions, name_serialized, 0, 0, Serialize::MemoryType_Auto, data, + dimensions, name_serialized, 0, 0, Serialize::MemoryType_Auto, scalar_data, scalar_default_serialized.first, scalar_default_serialized.second, scalar_min_serialized.first, scalar_min_serialized.second, scalar_max_serialized.first, scalar_max_serialized.second, diff --git a/src/halide_ir.fbs b/src/halide_ir.fbs index cab57a7dc044..685d20995761 100644 --- a/src/halide_ir.fbs +++ b/src/halide_ir.fbs @@ -618,7 +618,7 @@ table Parameter { host_alignment: int32; buffer_constraints: [BufferConstraint]; memory_type: MemoryType; - data: uint64; + scalar_data: uint64 = null; // Note: it is valid for this to be omitted, even if is_buffer = false. scalar_default: Expr; scalar_min: Expr; scalar_max: Expr; diff --git a/test/correctness/func_clone.cpp b/test/correctness/func_clone.cpp index c5cfbe868b61..8bf0b4d80e87 100644 --- a/test/correctness/func_clone.cpp +++ b/test/correctness/func_clone.cpp @@ -159,6 +159,7 @@ int update_defined_after_clone_test() { return 1; } + param.set(false); Buffer im = g.realize({200, 200}); auto func = [](int x, int y) { return ((0 <= x && x <= 99) && (0 <= y && y <= 99) && (x < y)) ? 3 * (x + y) : (x + y); diff --git a/test/correctness/specialize.cpp b/test/correctness/specialize.cpp index 54c74ca8ada9..1a807003f72a 100644 --- a/test/correctness/specialize.cpp +++ b/test/correctness/specialize.cpp @@ -447,6 +447,8 @@ int main(int argc, char **argv) { Buffer input(3, 3), output(3, 3); // Shouldn't throw a bounds error: im.set(input); + cond1.set(false); + cond2.set(false); out.realize(output); if (if_then_else_count != 1) { @@ -476,6 +478,8 @@ int main(int argc, char **argv) { Buffer input(3, 3), output(3, 3); // Shouldn't throw a bounds error: im.set(input); + cond1.set(false); + cond2.set(false); out.realize(output); // There should have been 2 Ifs total: They are the diff --git a/test/error/CMakeLists.txt b/test/error/CMakeLists.txt index 6e69490657f5..dde9d35fea0b 100644 --- a/test/error/CMakeLists.txt +++ b/test/error/CMakeLists.txt @@ -107,6 +107,8 @@ tests(GROUPS error undefined_pipeline_compile.cpp undefined_pipeline_realize.cpp undefined_rdom_dimension.cpp + uninitialized_param.cpp + uninitialized_param_2.cpp unknown_target.cpp vector_tile.cpp vectorize_dynamic.cpp diff --git a/test/error/uninitialized_param.cpp b/test/error/uninitialized_param.cpp new file mode 100644 index 000000000000..2fa8d47b2ec3 --- /dev/null +++ b/test/error/uninitialized_param.cpp @@ -0,0 +1,22 @@ +#include "Halide.h" +#include + +using namespace Halide; + +int main(int argc, char **argv) { + ImageParam image_param(Int(32), 2, "image_param"); + Param scalar_param("scalar_param"); + + Var x("x"), y("y"); + Func f("f"); + + f(x, y) = image_param(x, y) + scalar_param; + + Buffer b(10, 10); + image_param.set(b); + + f.realize({10, 10}); + + printf("Success!\n"); + return 0; +} diff --git a/test/error/uninitialized_param_2.cpp b/test/error/uninitialized_param_2.cpp new file mode 100644 index 000000000000..1f4eed42027a --- /dev/null +++ b/test/error/uninitialized_param_2.cpp @@ -0,0 +1,46 @@ +#include "Halide.h" +#include "halide_test_dirs.h" +#include + +using namespace Halide; +using namespace Halide::ConciseCasts; + +#include "Halide.h" + +namespace { + +Var x; + +class PleaseFail : public Halide::Generator { +public: + Input> input{"input"}; + Input scalar_input{"scalar_input"}; + Output> output{"output"}; + + void generate() { + Func lut_fn("lut_fn"); + lut_fn(x) = u8_sat(x * scalar_input / 255.f); + + // This should always fail, because it depends on a scalar input + // that *cannot* have a valid value at this point. + auto lut = lut_fn.realize({256}); + + output(x) = input(x) + lut[0](x); + } +}; + +} // namespace + +HALIDE_REGISTER_GENERATOR(PleaseFail, PleaseFail) + +int main(int argc, char **argv) { + Halide::Internal::ExecuteGeneratorArgs args; + args.output_dir = Internal::get_test_tmp_dir(); + args.output_types = std::set{OutputFileType::object}; + args.targets = std::vector{get_target_from_environment()}; + args.generator_name = "PleaseFail"; + execute_generator(args); + + printf("Success!\n"); + return 0; +}