Issues reported by Coverity

[henricj]

A GitHub workflow runs Coverity for another project that uses adplug as a submodule. These were reported as new defects yesterday (I have not reviewed them):

Hi,

Please find the latest report on new defect(s) introduced to henricj/dunelegacy found with Coverity Scan.

4 new defect(s) introduced to henricj/dunelegacy found with Coverity Scan.
2 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.

New defect(s) Reported-by: Coverity Scan
Showing 4 of 4 defect(s)

** CID 300174:  Uninitialized members  (UNINIT_CTOR)
/home/runner/work/dunelegacy/dunelegacy/external/adplug/adplug/src/coktel.h: 53 in CcoktelPlayer::CcoktelPlayer(Copl *)()

________________________________________________________________________________________________________
*** CID 300174:  Uninitialized members  (UNINIT_CTOR)
/home/runner/work/dunelegacy/dunelegacy/external/adplug/adplug/src/coktel.h: 53 in CcoktelPlayer::CcoktelPlayer(Copl *)()
47     {
48     public:
49      static CPlayer *factory(Copl *newopl);
50
51      CcoktelPlayer(Copl *newopl)
52              : CcomposerBackend(newopl), insts(0), data(0)
>>>     CID 300174:  Uninitialized members  (UNINIT_CTOR)
>>>     Non-static class member "modifyTimbre" is not initialized in this constructor nor in any functions that it calls.
53              { }
54      ~CcoktelPlayer()
55      {
56              if (insts) delete [] insts;
57              if (data) delete [] data;
58      };

** CID 300173:  Security best practices violations  (STRING_OVERFLOW)
/home/runner/work/dunelegacy/dunelegacy/external/adplug/adplug/src/composer.cpp: 520 in CcomposerBackend::load_bnk_instrument(binistream *, const CcomposerBackend::SBnkHeader &, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &)()

________________________________________________________________________________________________________
*** CID 300173:  Security best practices violations  (STRING_OVERFLOW)
/home/runner/work/dunelegacy/dunelegacy/external/adplug/adplug/src/composer.cpp: 520 in CcomposerBackend::load_bnk_instrument(binistream *, const CcomposerBackend::SBnkHeader &, const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> &)()
514
515         char ncs[INS_MAX_NAME_SIZE];
516         if (header.case_sensitive)
517         {
518             // assuming a bank with case sensitive names stores them in uppercase
519             // this is true for implay.bnk at least
>>>     CID 300173:  Security best practices violations  (STRING_OVERFLOW)
>>>     You might overrun the 9-character fixed-size string "ncs" by copying the return value of "c_str" without checking the length.
520             strcpy(ncs, name.c_str());
521             strup(ncs);
522         }
523
524         typedef TInstrumentNames::const_iterator TInsIter;
525         typedef std::pair<TInsIter, TInsIter>    TInsIterPair;

** CID 300172:  Uninitialized members  (UNINIT_CTOR)
/home/runner/work/dunelegacy/dunelegacy/external/adplug/adplug/src/composer.cpp: 158 in CcomposerBackend::CcomposerBackend(Copl *)()

________________________________________________________________________________________________________
*** CID 300172:  Uninitialized members  (UNINIT_CTOR)
/home/runner/work/dunelegacy/dunelegacy/external/adplug/adplug/src/composer.cpp: 158 in CcomposerBackend::CcomposerBackend(Copl *)()
152         , mKeyOnCache        (kNumPercussiveVoices, false)
153         , mOldPitchBendLength(~0)
154         , mPitchRangeStep    (skNrStepPitch)
155         , mOldHalfToneOffset (0)
156         , mAMVibRhythmCache  (0)
157     {
>>>     CID 300172:  Uninitialized members  (UNINIT_CTOR)
>>>     Non-static class member "mRhythmMode" is not initialized in this constructor nor in any functions that it calls.
158     }
159     //---------------------------------------------------------
160     void CcomposerBackend::rewind(int subsong)
161     {
162         mHalfToneOffset  = TInt16Vector(kNumPercussiveVoices, 0);
163         mVolumeCache     = TUInt8Vector(kNumPercussiveVoices, kMaxVolume);

** CID 300171:    (OVERRUN)

________________________________________________________________________________________________________
*** CID 300171:    (OVERRUN)
/home/runner/work/dunelegacy/dunelegacy/external/adplug/adplug/src/coktel.cpp: 198 in CcoktelPlayer::executeCommand()()
192                                     break;
193
194                             case COK_VOLUME_SLIDE:
195                                     val = data[pos++];
196                                     if (voice >= MAX_VOICES)
197                                             break;
>>>     CID 300171:    (OVERRUN)
>>>     Overrunning callee's array of size 9 by passing argument "voice" (which evaluates to 10) in call to "SetVolume".
198                                     SetVolume(voice, val);
199                                     break;
200
201                             case COK_TIMBRE_CHANGE:
202                                     val = data[pos++];
203                                     if (voice >= MAX_VOICES)
/home/runner/work/dunelegacy/dunelegacy/external/adplug/adplug/src/coktel.cpp: 210 in CcoktelPlayer::executeCommand()()
204                                             break;
205                                     if (insts)
206                                     {
207                                             if (val < nrTimbre)
208                                             {
209                                                     timbre[voice] = val;
>>>     CID 300171:    (OVERRUN)
>>>     Overrunning callee's array of size 9 by passing argument "voice" (which evaluates to 10) in call to "SetInstrument".
210                                                     SetInstrument(voice, insts[val].backend_index);
211                                             }
212                                             #ifdef DEBUG
213                                             else
214                                             {
215                                                     AdPlug_LogWrite("Timbre not found: %d\n", val);
/home/runner/work/dunelegacy/dunelegacy/external/adplug/adplug/src/coktel.cpp: 170 in CcoktelPlayer::executeCommand()()
164                     {
165                             case COK_NOTE_ON_VOL:
166                                     note = data[pos++];
167                                     val = data[pos++];
168                                     if (voice >= MAX_VOICES)
169                                             break;
>>>     CID 300171:    (OVERRUN)
>>>     Overrunning callee's array of size 9 by passing argument "voice" (which evaluates to 10) in call to "SetVolume".
170                                     SetVolume(voice, val);
171                                     NoteOn(voice, note);
172                                     break;
173
174                             case COK_NOTE_OFF:
175                                     if (voice >= MAX_VOICES)

[mywave82]

modifyTimbre is set to 0xff in this path

1. CcoktelPlayer::load()
2. CcomposerBackend::rewind()
3. CcoktelPlayer::frontend_rewind() Adding explicit default value in ctor is not needed, but does not hurt either.

mRhythmMode is initalized in each playback path like this example:

1. CcoktelPlayer::load()
2. CcomposerBackend::rewind()
3. CcoktelPlayer::frontend_rewind()
4. CcomposerBackend::SetRhythmMode()

load_bnk_instrument(): strcpy(ncs, name.c_str()); the current two callers that reach have string limited, but adding an assertion or protection does not hurt

The array overflows I am not sure why the Coverity complains about. kNumPercussiveVoices == MAX_VOICES == 11 SetVolume() can receive voice 0-10 SetInstrument() => send_operator() can receive voice 0-11

They are all false positives as far as I can see, but for some of them we can add stuff to the code that does not have negative impact