Coverity finding CID 170385, Uninitialized scalar field (UNINIT_CTOR)

[noloader]

Coverity finding CID 170385 (1 of 1): Uninitialized scalar field (UNINIT_CTOR) in integer.cpp:

342  #ifdef CRYPTOPP_NATIVE_DWORD_AVAILABLE
   1. member_decl: Class member declaration for m_whole.
343          dword m_whole;
344  #else

1. uninit_member: Non-static class member m_whole is not initialized in this constructor nor in any functions that it calls.***

235        DWord(word low, word high)
236        {
237                m_halfs.low = low;
238                m_halfs.high = high;
239        }

In the call ctor above, m_whole and m_halfs are different members of the same union. While Coverity is complaining we did not initialize m_whole because NATIVE_DWORD is in effect, there's a bigger problem lurking: undefined behavior when accessing the inactive union members (m_halves.low and m_halves.high).

The fix for UNINIT_CTOR finding below makes the undefined behavior more apparent.

[noloader]

Here's the fix. Keep in mind the union looks like this:

union
{
#ifdef CRYPTOPP_NATIVE_DWORD_AVAILABLE
    dword m_whole;
#endif
    half_words m_halfs;
};

And the actual fix for the UNINIT_CTOR finding. First, we value-initialized without punning (the m_whole() and m_halfs()). Then, we respected C++ union rules when assigning to m_whole and m_halfs.

#if defined(CRYPTOPP_NATIVE_DWORD_AVAILABLE)
DWord(word low, word high) : m_whole()
#else
DWord(word low, word high) : m_halfs()
#endif
{
#if defined(CRYPTOPP_NATIVE_DWORD_AVAILABLE)
#  if defined(IS_LITTLE_ENDIAN)
    const word t[2] = {low,high};
    memcpy(&m_whole, &t, sizeof(m_whole));
#  else
    const word t[2] = {high,low};
    memcpy(&m_whole, &t, sizeof(m_whole));
#  endif
#else
    m_halfs.low = low;
    m_halfs.high = high;
#endif
}

---

The extra gyrations below ensure we access through w_whole when NATIVE_DWORD is in effect:

    const word t[2] = {low,high};
    memcpy(&m_whole, &t, sizeof(m_whole));

The above is more succinctly expressed as m_whole = (((dword)low) << 64U) | high;. However, some compilers claim 64-bits is outside the range of [0-127] for the 128-bit integer.

[noloader]

Cleared at Commit 584f2f2ad11edb08.

There's some undefined behavior lurking here, around line 320:

// TODO: When NATIVE_DWORD is in effect, we access high and low, which are inactive
//  union members, and that's UB. Also see http://stackoverflow.com/q/11373203.
word GetLowHalf() const {return m_halfs.low;}
word GetHighHalf() const {return m_halfs.high;}
word GetHighHalfAsBorrow() const {return 0-m_halfs.high;}

---

Issue 31, BlumBlumShub validation fails under ARMEL, had this problem (exchanged in private emails with one of our maintainers and a GCC dev):

u = (D) A[1] - p.GetHighHalf() - u.GetHighHalfAsBorrow() - D::Multiply(B1, Q);

Yeah, this is definitely the sore spot.... Using real data, the expression above evaluates to:

u = 93 - 0 - 0 - 0

On ARM64, the result is 18446744073709551615. Ouch!

If I get a whiff that GetLowHalf, GetHighHalf or GetHighHalfAsBorrow are misbehaving, then they are next. We selected the ctor problem because of the Coverity finding, and the Covertiy finding on the uninitialized member explains the 18446744073709551615.