From 42d45cee1e8f74b24db8d4c8145624d509c08cf0 Mon Sep 17 00:00:00 2001 From: Shahram Najm Date: Mon, 8 Jan 2024 17:39:10 +0000 Subject: [PATCH 1/4] ECC-1741: Assertion failure: Encoding a large field in GRIB1 --- src/eccodes_prototypes.h | 2 +- src/grib_accessor_class.cc | 3 ++- src/grib_accessor_class_data_g1simple_packing.cc | 3 ++- src/grib_accessor_class_g1_message_length.cc | 1 + src/grib_accessor_class_g1bitmap.cc | 3 ++- src/grib_buffer.cc | 8 +++++--- src/grib_value.cc | 4 ++-- tests/grib_png.sh | 2 +- 8 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/eccodes_prototypes.h b/src/eccodes_prototypes.h index ba88a265f..aba20c9b4 100644 --- a/src/eccodes_prototypes.h +++ b/src/eccodes_prototypes.h @@ -801,7 +801,7 @@ void grib_grow_buffer(const grib_context* c, grib_buffer* b, size_t new_size); void grib_buffer_set_ulength_bits(const grib_context* c, grib_buffer* b, size_t length_bits); void grib_buffer_set_ulength(const grib_context* c, grib_buffer* b, size_t length); void grib_recompute_sections_lengths(grib_section* s); -void grib_buffer_replace(grib_accessor* a, const unsigned char* data, size_t newsize, int update_lengths, int update_paddings); +int grib_buffer_replace(grib_accessor* a, const unsigned char* data, size_t newsize, int update_lengths, int update_paddings); void grib_update_sections_lengths(grib_handle* h); /* grib_dumper.cc*/ diff --git a/src/grib_accessor_class.cc b/src/grib_accessor_class.cc index 06da7e830..524e40ecf 100644 --- a/src/grib_accessor_class.cc +++ b/src/grib_accessor_class.cc @@ -322,7 +322,8 @@ int grib_section_adjust_sizes(grib_section* s, int update, int depth) if (update) { plen = length; lret = grib_pack_long(s->aclength, &plen, &len); - Assert(lret == GRIB_SUCCESS); + if (lret != GRIB_SUCCESS) + return lret; s->padding = 0; } else { diff --git a/src/grib_accessor_class_data_g1simple_packing.cc b/src/grib_accessor_class_data_g1simple_packing.cc index 0541388f6..c863f7595 100644 --- a/src/grib_accessor_class_data_g1simple_packing.cc +++ b/src/grib_accessor_class_data_g1simple_packing.cc @@ -313,7 +313,8 @@ static int pack_double(grib_accessor* a, const double* cval, size_t* len) grib_context_log(a->context, GRIB_LOG_DEBUG, "grib_accessor_data_g1simple_packing : pack_double : packing %s, %d values", a->name, n_vals); - grib_buffer_replace(a, buf, buflen, 1, 1); + ret = grib_buffer_replace(a, buf, buflen, 1, 1); + if (ret != GRIB_SUCCESS) return ret; grib_context_buffer_free(a->context, buf); diff --git a/src/grib_accessor_class_g1_message_length.cc b/src/grib_accessor_class_g1_message_length.cc index 630202579..44e2f84bc 100644 --- a/src/grib_accessor_class_g1_message_length.cc +++ b/src/grib_accessor_class_g1_message_length.cc @@ -210,6 +210,7 @@ static int pack_long(grib_accessor* a, const long* val, size_t* len) " (actual length=%ld)", cclass_name, __func__, *val, total_length); grib_context_log(a->context, GRIB_LOG_ERROR, "Hint: Try encoding as GRIB2\n"); + return GRIB_ENCODING_ERROR; } Assert(total_length == *val); } diff --git a/src/grib_accessor_class_g1bitmap.cc b/src/grib_accessor_class_g1bitmap.cc index bfa835f01..b624d4dbe 100644 --- a/src/grib_accessor_class_g1bitmap.cc +++ b/src/grib_accessor_class_g1bitmap.cc @@ -149,7 +149,8 @@ static int pack_double(grib_accessor* a, const double* val, size_t* len) if ((err = grib_set_long_internal(grib_handle_of_accessor(a), self->unusedBits, tlen * 8 - *len)) != GRIB_SUCCESS) return err; - grib_buffer_replace(a, buf, tlen, 1, 1); + err = grib_buffer_replace(a, buf, tlen, 1, 1); + if (err) return err; grib_context_free(a->context, buf); diff --git a/src/grib_buffer.cc b/src/grib_buffer.cc index 43e7f7bc3..525e59f31 100644 --- a/src/grib_buffer.cc +++ b/src/grib_buffer.cc @@ -195,8 +195,8 @@ static void update_offsets_after(grib_accessor* a, long len) // update_sections_lengths(s->owner->parent); // } -void grib_buffer_replace(grib_accessor* a, const unsigned char* data, - size_t newsize, int update_lengths, int update_paddings) +int grib_buffer_replace(grib_accessor* a, const unsigned char* data, + size_t newsize, int update_lengths, int update_paddings) { size_t offset = a->offset; long oldsize = grib_get_next_position_offset(a) - offset; @@ -232,11 +232,13 @@ void grib_buffer_replace(grib_accessor* a, const unsigned char* data, update_offsets_after(a, increase); if (update_lengths) { grib_update_size(a, newsize); - grib_section_adjust_sizes(grib_handle_of_accessor(a)->root, 1, 0); + int err = grib_section_adjust_sizes(grib_handle_of_accessor(a)->root, 1, 0); + if (err) return err; if (update_paddings) grib_update_paddings(grib_handle_of_accessor(a)->root); } } + return GRIB_SUCCESS; } void grib_update_sections_lengths(grib_handle* h) diff --git a/src/grib_value.cc b/src/grib_value.cc index 538e73c2a..2ac9cb3ce 100644 --- a/src/grib_value.cc +++ b/src/grib_value.cc @@ -755,7 +755,7 @@ int grib_set_double_array_internal(grib_handle* h, const char* name, const doubl } if (ret != GRIB_SUCCESS) - grib_context_log(h->context, GRIB_LOG_ERROR, "Unable to set double array %s (%s)", + grib_context_log(h->context, GRIB_LOG_ERROR, "Unable to set double array '%s' (%s)", name, grib_get_error_message(ret)); /*if (h->context->debug) fprintf(stderr,"ECCODES DEBUG grib_set_double_array_internal key=%s --DONE\n",name);*/ return ret; @@ -928,7 +928,7 @@ int grib_set_long_array_internal(grib_handle* h, const char* name, const long* v { int ret = _grib_set_long_array(h, name, val, length, 0); if (ret != GRIB_SUCCESS) - grib_context_log(h->context, GRIB_LOG_ERROR, "Unable to set long array %s (%s)", + grib_context_log(h->context, GRIB_LOG_ERROR, "Unable to set long array '%s' (%s)", name, grib_get_error_message(ret)); return ret; } diff --git a/tests/grib_png.sh b/tests/grib_png.sh index eee61eec5..6ad01ed3e 100755 --- a/tests/grib_png.sh +++ b/tests/grib_png.sh @@ -53,7 +53,7 @@ infile=${data_dir}/sample.grib2 set +e ${tools_dir}/grib_set -r -s packingType=grid_png $infile $temp > $tempErr 2>&1 set -e -grep -q "Unable to set double array codedValues" $tempErr +grep -q "Unable to set double array 'codedValues'" $tempErr # Nearest neighbour # ---------------------- From bfda78d07ce894e0e3962c2477af34dd310099e6 Mon Sep 17 00:00:00 2001 From: Shahram Najm Date: Mon, 8 Jan 2024 18:20:27 +0000 Subject: [PATCH 2/4] ECC-1741: Add test --- tests/CMakeLists.txt | 2 + tests/grib_set_large_message_fail.cc | 65 ++++++++++++++++++++++++++++ tests/grib_set_large_message_fail.sh | 14 ++++++ 3 files changed, 81 insertions(+) create mode 100644 tests/grib_set_large_message_fail.cc create mode 100755 tests/grib_set_large_message_fail.sh diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 94bbc2882..3bed4fc84 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -19,6 +19,7 @@ list(APPEND test_c_bins grib_multi_from_message grib_clone_headers_only grib_read_index + grib_set_large_message_fail unit_tests bufr_keys_iter grib_keys_iter @@ -160,6 +161,7 @@ if( HAVE_BUILD_TOOLS ) # and/or take much longer list(APPEND tests_extra grib_data_quality_checks + grib_set_large_message_fail grib_g1monthlydate grib_g1fcperiod grib_bpv_limit diff --git a/tests/grib_set_large_message_fail.cc b/tests/grib_set_large_message_fail.cc new file mode 100644 index 000000000..ea0b2a2c9 --- /dev/null +++ b/tests/grib_set_large_message_fail.cc @@ -0,0 +1,65 @@ +/* + * (C) Copyright 2005- ECMWF. + * + * This software is licensed under the terms of the Apache Licence Version 2.0 + * which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. + * + * In applying this licence, ECMWF does not waive the privileges and immunities granted to it by + * virtue of its status as an intergovernmental organisation nor does it submit to any jurisdiction. + */ + +#include +#include +#undef NDEBUG +#include + +#include "eccodes.h" + +int main(int argc, char** argv) +{ + int err = 0, i = 0, NUM_MISSING = 10; + codes_handle* h = NULL; + size_t values_len = 0; + long Ni = 0, Nj = 0; + double* values = NULL; + const double missing = 1.0e36; + bool use_bitmap = true; + + h = codes_grib_handle_new_from_samples(NULL, "GRIB1"); + + CODES_CHECK(codes_set_double(h, "missingValue", missing), 0); + + Ni = Nj = 20000; + values_len = Ni * Nj; + + values = (double*)calloc(values_len, sizeof(double)); + + CODES_CHECK(codes_set_long(h, "Ni", Ni), 0); + CODES_CHECK(codes_set_long(h, "Nj", Nj), 0); + + if (use_bitmap) { + printf("Adding a bitmap...\n"); + CODES_CHECK(codes_set_long(h, "bitmapPresent", 1), 0); + for (i = 0; i < NUM_MISSING; i++) { + values[i] = missing; + } + } else { + printf("Not adding a bitmap...\n"); + values[0] = 42; + values[1] = 52; + } + + printf("Setting the values array...\n"); + err = codes_set_double_array(h, "values", values, values_len); + if (err) { + printf("codes_set_double_array failed as expected: err=%s\n", codes_get_error_message(err)); + } else { + fprintf(stderr, "codes_set_double_array should have failed!\n"); + return 1; + } + + codes_handle_delete(h); + free(values); + + return 0; +} diff --git a/tests/grib_set_large_message_fail.sh b/tests/grib_set_large_message_fail.sh new file mode 100755 index 000000000..fbee28b36 --- /dev/null +++ b/tests/grib_set_large_message_fail.sh @@ -0,0 +1,14 @@ +#!/bin/sh +# (C) Copyright 2005- ECMWF. +# +# This software is licensed under the terms of the Apache Licence Version 2.0 +# which can be obtained at http://www.apache.org/licenses/LICENSE-2.0. +# +# In applying this licence, ECMWF does not waive the privileges and immunities granted to it by +# virtue of its status as an intergovernmental organisation nor does it submit to any jurisdiction. +# + +. ./include.ctest.sh + +$EXEC ${test_dir}/grib_set_large_message_fail + From b367af2fca04ef9cab608b19c799d57038ac5cfd Mon Sep 17 00:00:00 2001 From: Shahram Najm Date: Mon, 8 Jan 2024 18:28:16 +0000 Subject: [PATCH 3/4] ECC-1741: Test with and without bitmap --- tests/grib_set_large_message_fail.cc | 9 +++++++-- tests/grib_set_large_message_fail.sh | 11 ++++++++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/tests/grib_set_large_message_fail.cc b/tests/grib_set_large_message_fail.cc index ea0b2a2c9..599d017be 100644 --- a/tests/grib_set_large_message_fail.cc +++ b/tests/grib_set_large_message_fail.cc @@ -23,9 +23,14 @@ int main(int argc, char** argv) long Ni = 0, Nj = 0; double* values = NULL; const double missing = 1.0e36; - bool use_bitmap = true; + bool use_bitmap = false; + + if (argc == 2 && strcmp(argv[1], "-b")==0) { + use_bitmap = true; + } h = codes_grib_handle_new_from_samples(NULL, "GRIB1"); + assert(h); CODES_CHECK(codes_set_double(h, "missingValue", missing), 0); @@ -54,7 +59,7 @@ int main(int argc, char** argv) if (err) { printf("codes_set_double_array failed as expected: err=%s\n", codes_get_error_message(err)); } else { - fprintf(stderr, "codes_set_double_array should have failed!\n"); + fprintf(stderr, "Error: codes_set_double_array should have failed!\n"); return 1; } diff --git a/tests/grib_set_large_message_fail.sh b/tests/grib_set_large_message_fail.sh index fbee28b36..9e8a4c775 100755 --- a/tests/grib_set_large_message_fail.sh +++ b/tests/grib_set_large_message_fail.sh @@ -10,5 +10,14 @@ . ./include.ctest.sh -$EXEC ${test_dir}/grib_set_large_message_fail +label='grib_set_large_message_fail_test' +temp=temp.$label.txt +$EXEC ${test_dir}/grib_set_large_message_fail > $temp 2>&1 +grep -q "Failed to set GRIB1 message length" $temp + +$EXEC ${test_dir}/grib_set_large_message_fail -b > $temp 2>&1 +grep -q "Unable to set double array.*bitmap" $temp + +# Clean up +rm -f $temp From adb1261b752f8b144b61ce00b6f9306ae8441c55 Mon Sep 17 00:00:00 2001 From: Shahram Najm Date: Mon, 8 Jan 2024 18:50:33 +0000 Subject: [PATCH 4/4] Remove deprecated file --- examples/python/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/python/CMakeLists.txt b/examples/python/CMakeLists.txt index 42d8a2c52..abbf8b21a 100644 --- a/examples/python/CMakeLists.txt +++ b/examples/python/CMakeLists.txt @@ -3,7 +3,7 @@ # Configure the file which all CMake tests will include configure_file( include.ctest.sh.in include.ctest.sh @ONLY ) -execute_process( COMMAND ${CMAKE_COMMAND} -E copy_if_different ${CMAKE_CURRENT_SOURCE_DIR}/include.sh ${CMAKE_CURRENT_BINARY_DIR} ) +# execute_process( COMMAND ${CMAKE_COMMAND} -E copy_if_different ${CMAKE_CURRENT_SOURCE_DIR}/include.sh ${CMAKE_CURRENT_BINARY_DIR} ) # Build the executables used by test scripts ################################################