Fixing a member (m_nHeader) being referenced before initialized

[maxsharabayko]

Fixed a member (m_nHeader) being referenced before initialized. Fixes #2400.

Status

Seems to help when compiled with -Werror and GCC 12 (see this comment).
(int32_t&) m_nHeader[SRT_PH_SEQNO] is still a problem. Even though m_nHeader[SRT_PH_SEQNO] is a valid address, dereferencing it worries the sanitizer because the value itself is not initialized.

It is not an error in this case but would be nice to have it resolved.

[jlsantiago0]

Hi @maxsharabayko Per your request, I tested SRT master branch and this branch IRT Issue #2400 . The Sanitizer error is the same in both cases:

[ 29%] Building CXX object CMakeFiles/srt_virtual.dir/srtcore/packet.cpp.o
/mnt/share/open/srt/srt/srtcore/packet.cpp: In constructor ?srt::CPacket::CPacket()?:
/mnt/share/open/srt/srt/srtcore/packet.cpp:180:25: warning: member ?srt::CPacket::m_nHeader? is used uninitialized [-Wuninitialized]
  180 |     m_iSeqNo((int32_t&)(m_nHeader[SRT_PH_SEQNO])),
      |                         ^~~~~~~~~

[jlsantiago0]

NOTE: This reads from a memory address that has not been set. It is not too much of a problem, unless this memory location happens to reside in a virtual page frame that has not been mapped to a physical page frame, then it can segfault. This can happen when for instance a new page frame is created, but has never been written too. Reading from a memory address there can segfault. Either way it is undefined behavior.

Perhaps m_iSeqNo could be set to NULL here and then m_nHeader could be initialized in the constructor body and then the m_iSeqNo could be set afterwards? This would solve two purposes, it would ensure that m_iSeqNo is initialized with a known value at that point and you wont have the potential of a segfault in this (admittedly unlikely) case. But unlikely cases tend to happen with enough instances.

[ethouris]

This is because the m_nHeader[SRT_PH_SEQNO] is indeed not initialized. But I don't understand why the sanitizer sees a problem there, while it's not the value, but the reference (through the pointer) being initialized here, so the initialization status of m_nHeader[SRT_PH_SEQNO] shouldn't matter. It should matter when someone is trying to read the m_iSeqNo field.

OTOH I frankly never liked these reference fields. I have once tried to refax them to turn these names into methods returning ad-hoc references to these fields (or, this time, by using the PROPERTIES through SRTU_PROPERTY_RW macro).

[ethouris]

Perhaps m_iSeqNo could be set to NULL here and then m_nHeader could be initialized in the constructor body and then the m_iSeqNo could be set afterwards?

No because it's a reference. References can't be set to NULL, and tricks to do it are even more dangerous.

[jlsantiago0]

Perhaps m_iSeqNo could be set to NULL here and then m_nHeader could be initialized in the constructor body and then the m_iSeqNo could be set afterwards?

No because it's a reference. References can't be set to NULL, and tricks to do it are even more dangerous.

I think you can use the default constructor for m_iSeqNo and then assign it in the body after m_nHeader is in a valid state. You can reassign references. This should be fine as long as you don't try to use the reference until you have assigned it to a valid object.

[maxsharabayko]

Hmm, this PR actually helps to resolve the issue on GCC 12.0.1.

Configure

cmake .. -DENABLE_CODE_COVERAGE=ON -DENABLE_BONDING=ON -DCMAKE_CXX_FLAGS="-Werror" -DCMAKE_BUILD_TYPE=Debug

Build (Before the Fix)

[ 27%] Building CXX object CMakeFiles/srt_virtual.dir/srtcore/packet.cpp.o
/mnt/d/Projects/srt/srt-max/srtcore/packet.cpp: In constructor ‘srt::CPacket::CPacket()’:
/mnt/d/Projects/srt/srt-max/srtcore/packet.cpp:181:27: error: member ‘srt::CPacket::m_nHeader’ is used uninitialized [-Werror=uninitialized]
  181 |     , m_iSeqNo((int32_t&)(m_nHeader[SRT_PH_SEQNO]))
      |                           ^~~~~~~~~
cc1plus: all warnings being treated as errors
make[2]: *** [CMakeFiles/srt_virtual.dir/build.make:300: CMakeFiles/srt_virtual.dir/srtcore/packet.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:95: CMakeFiles/srt_virtual.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

After the Fix

All built successfully.

[codecov-commenter]

Codecov Report

Merging #2433 (50cf958) into master (fdb9389) will increase coverage by 0.42%.
The diff coverage is 45.16%.

❗ Current head 50cf958 differs from pull request most recent head afec211. Consider uploading reports for the commit afec211 to get more accurate results

@@            Coverage Diff             @@
##           master    #2433      +/-   ##
==========================================
+ Coverage   65.91%   66.33%   +0.42%     
==========================================
  Files         100      100              
  Lines       19822    19795      -27     
==========================================
+ Hits        13065    13131      +66     
+ Misses       6757     6664      -93     

Impacted Files	Coverage Δ
srtcore/logging.h	98.21% <ø> (+3.76%)	⬆️
srtcore/core.cpp	60.83% <40.74%> (+0.93%)	⬆️
srtcore/api.cpp	54.70% <66.66%> (+1.42%)	⬆️
srtcore/packet.cpp	44.04% <100.00%> (ø)
srtcore/common.cpp	37.32% <0.00%> (-0.93%)	⬇️
srtcore/queue.cpp	80.56% <0.00%> (-0.75%)	⬇️
srtcore/congctl.cpp	81.34% <0.00%> (+0.51%)	⬆️
test/test_bonding.cpp	98.42% <0.00%> (+0.52%)	⬆️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ethouris]

This is only a formal compiler shut-up. The thing taken from these uninitialized fields is their reference, which remains the same after initialization. Moreover, this type doesn't feature an explicit constructor that would initialize the internal array field, which means that it is "default initialized" (not default-constructed), i. e. left with garbage. This is cleared in the call of clear() method, which is called later in this constructor, but it happens already after the reference was taken. Only becaue this type contains methods, it can't be treated as POD, which means that it is not treated as initialized if there was no constructor call. This is actually a bit strange because such a type already has a constructor (the default one) and as the field is defined earlier than the reference fields, the compiler should have called this constructor automatically, as this field provides a (generated, but still) default constructor.

At least this initialization entry deserves some comment that explains this in short.