Skip to content

ECC-2143: Fix BUFR data races #455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
joobog wants to merge 3 commits into
base: develop
Choose a base branch
from
+73 −18
Draft

ECC-2143: Fix BUFR data races #455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4bbb0ec
ECC-2143: Partially fix OMP data races. More issues are addressed in …
joobog Apr 1, 2026
93f84c3
ECC-2143: Fix ThreadSanitizer macro
joobog Apr 1, 2026
5198820
ECC-2143: Fix memory leak
joobog Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 53 additions & 13 deletions src/eccodes/accessor/BufrDataArray.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,9 @@ int check_end_data(grib_context* c, bufr_descriptor* bd, BufrDataArray* self, in
void BufrDataArray::self_clear()
{
grib_context_free(context_, canBeMissing_);
grib_context_free(context_, nokeys_);
grib_context_free(context_, refOverrides_);
grib_context_free(context_, hasRefOverride_);
grib_vdarray_delete_content(numericValues_);
grib_vdarray_delete(numericValues_);

Expand Down Expand Up @@ -355,6 +358,12 @@ int BufrDataArray::get_descriptors()
numberOfDescriptors = grib_bufr_descriptors_array_used_size(expanded_);
if (canBeMissing_) grib_context_free(c, canBeMissing_);
canBeMissing_ = (int*)grib_context_malloc_clear(c, numberOfDescriptors * sizeof(int));
if (nokeys_) grib_context_free(c, nokeys_);
nokeys_ = (int*)grib_context_malloc_clear(c, numberOfDescriptors * sizeof(int));
if (refOverrides_) grib_context_free(c, refOverrides_);
refOverrides_ = (long*)grib_context_malloc_clear(c, numberOfDescriptors * sizeof(long));
if (hasRefOverride_) grib_context_free(c, hasRefOverride_);
hasRefOverride_ = (int*)grib_context_malloc_clear(c, numberOfDescriptors * sizeof(int));
for (i = 0; i < numberOfDescriptors; i++)
canBeMissing_[i] = grib_bufr_descriptor_can_be_missing(expanded_->v[i]);

Expand Down Expand Up @@ -942,7 +951,7 @@ int decode_element(grib_context* c, BufrDataArray* self, int subsetIndex,
number_of_bits, (long)*pos, (long)(*pos - self->offset_ * 8));
grib_context_log(c, GRIB_LOG_DEBUG, "Operator 203YYY: Store for code %6.6ld => new ref val %ld", bd->code, new_ref_val);
self->tableB_override_store_ref_val(c, bd->code, new_ref_val);
bd->nokey = 1;
self->nokeys_[i] = 1;
err = check_end_data(c, NULL, self, number_of_bits); /*advance bitsToEnd*/
return err;
}
Expand Down Expand Up @@ -978,9 +987,16 @@ int decode_element(grib_context* c, BufrDataArray* self, int subsetIndex,
}
else {
/* numeric or codetable or flagtable */
/* Operator 203YYY: Check if we have changed ref value for this element. If so modify bd->reference */
if (self->change_ref_value_operand_ != 0 && self->tableB_override_get_ref_val(bd->code, &(bd->reference)) == GRIB_SUCCESS) {
grib_context_log(c, GRIB_LOG_DEBUG, "Operator 203YYY: For code %6.6ld, changed ref val: %ld", bd->code, bd->reference);
/* Operator 203YYY: Check if we have changed ref value for this element. If so use local copy */
bufr_descriptor bd_local;
long overridden_ref;
if (self->change_ref_value_operand_ != 0 && self->tableB_override_get_ref_val(bd->code, &overridden_ref) == GRIB_SUCCESS) {
grib_context_log(c, GRIB_LOG_DEBUG, "Operator 203YYY: For code %6.6ld, changed ref val: %ld", bd->code, overridden_ref);
bd_local = *bd;
bd_local.reference = overridden_ref;
bd = &bd_local;
self->hasRefOverride_[i] = 1;
self->refOverrides_[i] = overridden_ref;
}

if (bd->width > 64) {
Expand Down Expand Up @@ -1125,6 +1141,7 @@ int BufrDataArray::encode_overridden_reference_value(grib_context* c, grib_buffe
currRefVal, bd->shortName, bd->code);
}
refValIndex_++;
tableB_override_store_ref_val(c, bd->code, currRefVal);
return err;
}

Expand Down Expand Up @@ -1171,6 +1188,14 @@ int encode_new_element(grib_context* c, BufrDataArray* self, int subsetIndex,
}
else {
/* numeric or codetable or flagtable */
/* Operator 203YYY: Use overridden reference if available */
bufr_descriptor bd_new_local;
long overridden_ref;
if (self->change_ref_value_operand_ != 0 && self->tableB_override_get_ref_val(bd->code, &overridden_ref) == GRIB_SUCCESS) {
bd_new_local = *bd;
bd_new_local.reference = overridden_ref;
bd = &bd_new_local;
}
grib_context_log(c, GRIB_LOG_DEBUG, "BUFR data encoding: \t %s = %g",
bd->shortName, cdval);
if (bd->code == 31031)
Expand Down Expand Up @@ -1296,6 +1321,14 @@ int encode_element(grib_context* c, BufrDataArray* self, int subsetIndex,
}
else {
/* numeric or codetable or flagtable */
/* Operator 203YYY: Use overridden reference if available */
bufr_descriptor bd_enc_local;
long overridden_ref;
if (self->change_ref_value_operand_ != 0 && self->tableB_override_get_ref_val(bd->code, &overridden_ref) == GRIB_SUCCESS) {
bd_enc_local = *bd;
bd_enc_local.reference = overridden_ref;
bd = &bd_enc_local;
}
if (self->compressedData_) {
err = self->encode_numeric_array(c, buff, pos, bd, self->numericValues_->v[elementIndex]);
if (err) {
Expand Down Expand Up @@ -1829,8 +1862,6 @@ grib_accessor* BufrDataArray::create_accessor_from_descriptor(grib_accessor* att
elementAccessor->numberOfSubsets(numberOfSubsets_);
elementAccessor->subsetNumber(subset);

expanded_->v[idx]->a = accessor;

if (attribute) {
/* attribute->parent=accessor->parent; */
/*
Expand Down Expand Up @@ -1865,7 +1896,8 @@ grib_accessor* BufrDataArray::create_accessor_from_descriptor(grib_accessor* att
return NULL;
accessor->add_attribute(attribute, 0);

attribute = create_attribute_variable("reference", section, GRIB_TYPE_DOUBLE, 0, expanded_->v[idx]->reference, 0, flags);
attribute = create_attribute_variable("reference", section, GRIB_TYPE_DOUBLE, 0,
hasRefOverride_[idx] ? refOverrides_[idx] : expanded_->v[idx]->reference, 0, flags);
if (!attribute)
return NULL;
accessor->add_attribute(attribute, 0);
Expand Down Expand Up @@ -1914,7 +1946,6 @@ grib_accessor* BufrDataArray::create_accessor_from_descriptor(grib_accessor* att
return NULL;
accessor->add_attribute(attribute, 0);
}
expanded_->v[idx]->a = accessor;
break;
case 9:
set_creator_name(&creator, expanded_->v[idx]->code);
Expand Down Expand Up @@ -1953,7 +1984,8 @@ grib_accessor* BufrDataArray::create_accessor_from_descriptor(grib_accessor* att
return NULL;
accessor->add_attribute(attribute, 0);

attribute = create_attribute_variable("reference", section, GRIB_TYPE_DOUBLE, 0, expanded_->v[idx]->reference, 0, flags);
attribute = create_attribute_variable("reference", section, GRIB_TYPE_DOUBLE, 0,
hasRefOverride_[idx] ? refOverrides_[idx] : expanded_->v[idx]->reference, 0, flags);
if (!attribute)
return NULL;
accessor->add_attribute(attribute, 0);
Expand Down Expand Up @@ -2337,7 +2369,7 @@ int BufrDataArray::create_keys(long onlySubset, long startSubset, long endSubset
idx = compressedData_ ? elementsDescriptorsIndex_->v[0]->v[ide] : elementsDescriptorsIndex_->v[iss]->v[ide];

descriptor = expanded_->v[idx];
if (descriptor->nokey == 1) {
if (descriptor->nokey == 1 || nokeys_[idx] == 1) {
continue; /* Descriptor does not have an associated key e.g. inside op 203YYY */
}
elementFromBitmap = NULL;
Expand Down Expand Up @@ -2561,6 +2593,11 @@ int BufrDataArray::create_keys(long onlySubset, long startSubset, long endSubset
}
}
}
if (associatedFieldSignificanceAccessor) {
associatedFieldSignificanceAccessor->destroy(c);
delete associatedFieldSignificanceAccessor;
associatedFieldSignificanceAccessor = nullptr;
}
(void)extraElement;
return err;
}
Expand Down Expand Up @@ -2962,14 +2999,17 @@ int BufrDataArray::process_elements(int flag, long onlySubset, long startSubset,
break;

case 5: /* Signify character */
descriptors[i]->width = descriptors[i]->Y * 8;
descriptors[i]->type = BUFR_DESCRIPTOR_TYPE_STRING;
err = codec_element(c, this, iss, buffer, data, &pos, i, 0, elementIndex, dval, sval);
{
bufr_descriptor bd_signify = *(descriptors[i]);
bd_signify.width = descriptors[i]->Y * 8;
bd_signify.type = BUFR_DESCRIPTOR_TYPE_STRING;
err = codec_element(c, this, iss, buffer, data, &pos, i, &bd_signify, elementIndex, dval, sval);
if (err) return err;
if (flag != PROCESS_ENCODE)
grib_iarray_push(elementsDescriptorsIndex, i);
elementIndex++;
break;
}
case 62: // ECC-968: BUFR edition 0 operator! not in the WMO standard
case 22: /* Quality information follows */
if (descriptors[i]->Y == 0) {
Expand Down
3 changes: 3 additions & 0 deletions src/eccodes/accessor/BufrDataArray.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ class BufrDataArray : public Gen
bufr_descriptors_array* expanded_ = nullptr;
ExpandedDescriptors* expandedAccessor_ = nullptr;
int* canBeMissing_ = nullptr;
int* nokeys_ = nullptr;
long* refOverrides_ = nullptr;
int* hasRefOverride_ = nullptr;
long numberOfSubsets_ = 0;
long compressedData_ = 0;
grib_vdarray* numericValues_ = nullptr;
Expand Down
1 change: 0 additions & 1 deletion src/eccodes/grib_api_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,6 @@ struct bufr_descriptor
long reference;
long width;
int nokey; /* set if descriptor does not have an associated key */
grib_accessor* a;
};

struct bufr_descriptors_array
Expand Down
21 changes: 17 additions & 4 deletions src/eccodes/sync/OmpNestLock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
#pragma once

#include <omp.h>
// #include <sanitizer/tsan_interface.h>

#if defined(__has_feature)
#if __has_feature(thread_sanitizer)
#include <sanitizer/tsan_interface.h>
#define ECCODES_TSAN_ENABLED 1
#endif
#elif defined(__SANITIZE_THREAD__)
#include <sanitizer/tsan_interface.h>
#define ECCODES_TSAN_ENABLED 1
#endif

namespace eccodes::sync {

// Note:
// The GCC15 thread sanitiser doesn't fully support OpenMP.
// See commented calls to tsan...
// Use TSan annotations to inform it about synchronization.

class OmpNestLock {
public:
Expand All @@ -21,11 +30,15 @@ class OmpNestLock {

void lock() {
omp_set_nest_lock(&mutex_);
// __tsan_acquire(&mutex_);
#if ECCODES_TSAN_ENABLED
__tsan_acquire(&mutex_);
#endif
}

void unlock() {
// __tsan_release(&mutex_);
#if ECCODES_TSAN_ENABLED
__tsan_release(&mutex_);
#endif
omp_unset_nest_lock(&mutex_);
}
private:
Expand Down
Loading