Bug - Valgrind: Conditional jump or move depends on uninitialised value(s)

[dejanyy]

Running the latest HarfBuzz 2.6.4 under Valgrind reveals a bug caused by uninitialized values.

Valgrind reports the following:

==14410== Conditional jump or move depends on uninitialised value(s)
==14410== at 0x483C0B7: bcmp (vg_replace_strmem.c:1112)
==14410== by 0x63AF8E: hb_shape_plan_key_t::equal(hb_shape_plan_key_t const*) (in /home/test/testprog)
==14410== by 0x644E13: hb_shape_plan_create_cached2 (in /home/test/testprog)
==14410== by 0x645FAD: hb_shape_full (in /home/test/testprog)
==14410== by 0x62D1AB: LayoutEngine::layoutChars(CDTLEUnicodeArray const&, int, int, int, bool, float, float, LEErrorCode&) (in /home/test/testprog)

==14410== Uninitialised value was created by a stack allocation
==14410== at 0x644D6B: hb_shape_plan_create_cached2 (in /home/test/testprog)

After some investigation, I believe I found where the problem is, but not its root cause.

First, let's have a look at hb_shape_plan_key_t::equal, which is in hb-shape-plan.cc on line 148:

bool
hb_shape_plan_key_t::equal (const hb_shape_plan_key_t *other)
{
return hb_segment_properties_equal (&this->props, &other->props) &&
this->user_features_match (other) &&
#ifndef HB_NO_OT_SHAPE
this->ot.equal (&other->ot) && // <<---- Valgrind detects the issue here
#endif
this->shaper_func == other->shaper_func;
}

and, in particular, this call on line 154:

this->ot.equal (&other->ot)

If we look at hb-ot-shape.hh, line 52, we can see the equal function of the hb_ot_shape_plan_key_t struct is implemented as follows:

bool equal (const hb_ot_shape_plan_key_t *other)
{
return 0 == memcmp (this, other, sizeof (*this));
}

which is basically a memcmp call. Most likely the OS maps memcmp to bcmp, and this is precisely where Valgrind detects the problem. Based on this, I guess that some members of the 'this' or 'other' struct are not initialized and when the times comes to compare them, Valgrind reports that a "conditional jump or move depends on uninitialised value(s)".

But obviously Valgrind detects the problem late, the real issue is why some members of the 'this' and/or 'other' struct are not initialized. As far as I can tell, the struct only has one data member however:

unsigned int variations_index[2];

So my best guess is that variations_index is not completely initialized within 'this' and/or 'other' struct.

I hope this helps you confirm and track down the bug. I'm happy to try your fix when it's available and confirm the resolution of the problem.

[ebraminio]

All looks good except how we can replicate the valgrind issue? We have a CI bot running valgrind on some parts of the tests so like to have this also covered in a CI bot.

[ebraminio]

The only way that hb_ot_shape_plan_key_t::variations_index[2] can be uninitialized is hb_ot_shape_plan_key_t::init that calls hb_ot_layout_table_find_feature_variations that calls GSUBGPOS::find_variations_index is compiled with HB_NO_VAR

diff --git a/src/hb-ot-layout-gsubgpos.hh b/src/hb-ot-layout-gsubgpos.hh
index 2111b87e..90892814 100644
--- a/src/hb-ot-layout-gsubgpos.hh
+++ b/src/hb-ot-layout-gsubgpos.hh
@@ -3054,6 +3054,7 @@ struct GSUBGPOS
                              unsigned int *index) const
   {
 #ifdef HB_NO_VAR
+    *index = NOT_FOUND_INDEX;
     return false;
 #endif
     return (version.to_int () >= 0x00010001u ? this+featureVars : Null(FeatureVariations))

which of course should be fixed indeed, otherwise we have a hb_ot_shape_plan_key_t that isn't initialized which as hb_shape_plan_key_t::init means hb_shape_plan_key_t is also isn't initialized. Are you passing HB_NO_VAR to your build configuration?

[ebraminio]

Applied b143e34 anyway and waiting to see how you've reproduced it.

[dejanyy]

OK, a few things:

1 - Yes, I'm passing HB_NO_VAR. More precisely HB_LEAN (a standard HarfBuzz setting), which means HB_NO_VAR.

2 - Your fix doesn't compile (hb-ot-layout-gsubgpos.hh:3042:14: error: ‘NOT_FOUND_INDEX’ was not declared in this scope). However, *index = Index::NOT_FOUND_INDEX and *index = FeatureVariations::NOT_FOUND_INDEX both compile and remove the Valgrind warning.

3 - I'm not sure which version of the code is right:

*index = Index::NOT_FOUND_INDEX

or

*index = FeatureVariations::NOT_FOUND_INDEX

Please note that these may not be precisely the same:

struct Index : HBUINT16 {
static constexpr unsigned NOT_FOUND_INDEX = 0xFFFFu;

and

struct FeatureVariations {
static constexpr unsigned NOT_FOUND_INDEX = 0xFFFFFFFFu;

Looking at code in hb-ot-layout-gsubgpos.hh it looks like it should be *index = FeatureVariations::NOT_FOUND_INDEX however I'm not that familiar with the code to be sure. Please double check and advise.
Thanks!

[ebraminio]

Oh, thanks for the analysis! If you like to please submit it as a PR and if not will do in some hours :)

[ebraminio]

Applied. Thanks! :)