PVS-Studio VS IPP Samples - part 3

I cannot get rid of the urge to sell a PVS-Studio license to the developer team of Intel Performance Primitives Library :).

Of course, I'd like to have some sales in other Intel departments too, but the IPP developers seem closer to us. First, some of them live in Russia. Second, they are already using static analyzers in their work. Third, we already have some groundwork in this area. I mean that we already checked the IPP Samples project twice (first check, second check). It would be, of course, much more interesting to test the IPP library itself instead of the samples, but we don't have access to its source codes.

So, I'll resort to the standard method of advertising ourselves. We will continue to reanalyze IPP Samples from time to time to find new errors. It indicates that the PVS-Studio code analyzer is actively developing. But the greatest profit can be, of course, obtained through regular use of the analyzer, not single runs. Do not forget the capabilities of night runs and background analysis after compilation.

I will cite some of the errors detected by PVS-Studio v4.60. These are, of course, not all the odd fragments we've found. There are much more of them, but I cannot tell for sure if they have errors or not. At the same time, let it be the reason for IPP developers to install and try our tool. We have changed the trial model so that the tool now possesses its full functionality. So, you are welcome to try it.

Let's have a look at odd fragments in the IPP Samples code.

Case N1. Meaningless statement

AACStatus bsacdecSetNumChannels(Ipp32s channelConfiguration,
                                AACDec *state)
{
  state->com.m_channel_number = channelConfiguration;
  if (channelConfiguration == 7) {
    state->com.m_channel_number;
  }

  return AAC_OK;
}

PVS-Studio's diagnostic message: V607 Ownerless expression 'state->com.m_channel_number'. aac_dec aac_dec_api_fp.c 1404

The "state->com.m_channel_number;" statement is very odd. An assignment or something else must be missing here.

Case N2. Filling the virtual method table

There are rather many places in IPP Samples where memory is allocated for classes through the malloc() function and initialized through the memset() function. Maybe there's nothing bad about it, but I'm discomforted by the fact that these classes have virtual methods. Thus, I suspect that the virtual method table might be spoiled and something will go wrong.

For example, the _MediaDataEx class contains virtual functions:

virtual bool TryStrongCasting(....) const;
virtual bool TryWeakCasting(....) const;

And this is how an object of this class is created:

Status VC1Splitter::Init(SplitterParams& rInit)
{
  MediaDataEx::_MediaDataEx *m_stCodes;
  ...
  m_stCodes = (MediaDataEx::_MediaDataEx *)
    ippsMalloc_8u(
      START_CODE_NUMBER*2*sizeof(Ipp32s)+
      sizeof(MediaDataEx::_MediaDataEx));
  ...
  memset(m_stCodes, 0,
   (START_CODE_NUMBER*2*sizeof(Ipp32s)+
   sizeof(MediaDataEx::_MediaDataEx)));
  ...
}

PVS-Studio's diagnostic message: V598 The 'memset' function is used to nullify the fields of '_MediaDataEx' class. Virtual method table will be damaged by this. vc1_spl umc_vc1_spl.cpp 131

I don't know if there is a problem here, but we'd better warn about it.

The same thing can be seen in the following places:

V598 The 'memset' function is used to nullify the fields of '_MediaDataEx' class. Virtual method table will be damaged by this. vc1_dec umc_vc1_video_decoder.cpp 641 False

V598 The 'memset' function is used to nullify the fields of 'AVS_DISASSEMBLING_CONTEXT' class. Virtual method table will be damaged by this. avs_enc umc_avs_enc_slice.cpp 45

V598 The 'memset' function is used to nullify the fields of 'AVS_DISASSEMBLING_CONTEXT' class. Virtual method table will be damaged by this. avs_enc umc_avs_enc_slice.cpp 29

V598 The 'memset' function is used to nullify the fields of 'AVS_DISASSEMBLING_CONTEXT' class. Virtual method table will be damaged by this. avs_enc umc_avs_enc_slice.cpp 22

V598 The 'memcpy' function is used to copy the fields of 'AVSVideoEncoderParams' class. Virtual method table will be damaged by this. avs_enc umc_avs_enc.cpp 115

V598 The 'memset' function is used to nullify the fields of 'AVS_DECODING_CONTEXT' class. Virtual method table will be damaged by this. avs_dec umc_avs_dec_slice_init.cpp 65

V598 The 'memset' function is used to nullify the fields of 'AVS_DEBLOCKING_CONTEXT' class. Virtual method table will be damaged by this. avs_common umc_avs_slice.cpp 153

V598 The 'memset' function is used to nullify the fields of 'AVS_RECONSTRUCTING_CONTEXT' class. Virtual method table will be damaged by this. avs_common umc_avs_slice.cpp 133

V598 The 'memset' function is used to nullify the fields of 'AVS_DEBLOCKING_CONTEXT' class. Virtual method table will be damaged by this. avs_common umc_avs_slice.cpp 43

V598 The 'memset' function is used to nullify the fields of 'AVS_RECONSTRUCTING_CONTEXT' class. Virtual method table will be damaged by this. avs_common umc_avs_slice.cpp 42

V598 The 'memset' function is used to nullify the fields of 'AVS_DECODING_CONTEXT' class. Virtual method table will be damaged by this. avs_common umc_avs_slice.cpp 41

V598 The 'memset' function is used to nullify the fields of 'AVS_DEBLOCKING_CONTEXT' class. Virtual method table will be damaged by this. avs_common umc_avs_slice.cpp 32

V598 The 'memset' function is used to nullify the fields of 'AVS_RECONSTRUCTING_CONTEXT' class. Virtual method table will be damaged by this. avs_common umc_avs_slice.cpp 31

V598 The 'memset' function is used to nullify the fields of 'AVS_DECODING_CONTEXT' class. Virtual method table will be damaged by this. avs_common umc_avs_slice.cpp 30

Case N3. Checking pointers after use

We've found several fragments where a pointer is used first and then is checked for being a null pointer. Perhaps the pointer will never be null and the code works correctly all the time, but it's not good anyway.

For example:

VIDEO_DRV_CREATE_BUFFERS_FUNC(....)
{
  ...
  VideoDrvVideoMemInfo* drv_vm = &(driver->m_VideoMemInfo);
  ...
  if ((NULL == driver) || (NULL == bufs))
  {
    ERR_SET(VM_NULL_PTR, "null ptr");
  }
  ...
}

PVS-Studio's diagnostic message: V595 The 'driver' pointer was utilized before it was verified against nullptr. Check lines: 40, 46. video_renders drv.c 40

The pointer check here should be either moved up or removed at all if the "dreiver==NULL" condition is impossible.

Here are other identical code samples:

Ipp16s *pNewSpeech = encoderObj->stEncState.pSpeechPtrNew;
if (NULL==encoderObj || NULL==src || NULL ==dst )
  return APIGSMAMR_StsBadArgErr;

PVS-Studio's diagnostic message: V595 The 'encoderObj' pointer was utilized before it was verified against nullptr. Check lines: 296, 298. speech encgsmamr.c 296

m_pAVSCompressorParams = DynamicCast<AVSVideoEncoderParams> (pParams);
...  
m_qp = m_pAVSCompressorParams->m_iConstQuant;
// check error(s)
if (NULL == m_pAVSCompressorParams)
  return UMC_ERR_NULL_PTR;

PVS-Studio's diagnostic message: V595 The 'm_pAVSCompressorParams' pointer was utilized before it was verified against nullptr. Check lines: 88, 91. avs_enc umc_avs_enc_fusion_core.cpp 88

Case N4. Odd expressions with commas

There are a couple of fragments with odd commas ','. The first sample:

void GetIntraDCPredictors(VC1Context* pContext)
{
  DCPred.DC[13] = pC->DCBlkPred[5].DC,QurrQuant;
  ...
}

PVS-Studio's diagnostic message: V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. vc1_dec umc_vc1_dec_mb_com.cpp 370

It might be a misprint, or something is missing here.

The second sample:

V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. speech usc_dtmf.c 309

static int DTMF_16s(....)
{
  ...
  for (i = pIppTDParams->dtmf_fs, j = 0;
       i < dtmf_frame_size+pIppTDParams->dtmf_fs, j < nbytes;
       i++, j++)
}

PVS-Studio's diagnostic message: V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. speech usc_dtmf.c 309

This is a more interesting example than the previous one. The logical condition seems to be written incorrectly. The condition must have looked as follows:

i < dtmf_frame_size+pIppTDParams->dtmf_fs && j < nbytes

Case N5. Odd implicit type conversion

class MeMV
{
public:
  MeMV(){};
  MeMV(int a0){x = (Ipp16s)a0; y=(Ipp16s)a0;};
  MeMV(int a0, int a1){x = (Ipp16s)a0; y=(Ipp16s)a1;};
  ...
}

MeMV MePredictCalculatorVC1::GetPrediction8x8()
{
  ...
  default:
    return false;
  ...
}

PVS-Studio's diagnostic message: V601 The 'false' value becomes a class object. me umc_vec_prediction.cpp 754

The GetPrediction8x8() function returns the MeMV type. But in one branch, it returns the 'false' value. This value is implicitly cast to 'int' and the MeMV(int a0) constructor is called. I'm not sure, but there is something else to be returned in this code, or an exception should be thrown.

An identical implicit type conversion can be found here:

V601 The 'false' value becomes a class object. me umc_vec_prediction.cpp 717

Case N6. Undefined behavior

In very many places of IPP Samples, you can find constructs that cause undefined or unspecified behavior. I wrote about some of them in the previous post. Now we have found a whole lot of negative value shifts. I cannot tell for sure that it may cause some troubles, but I recommend considering this article: "Wade not in unknown waters. Part three" just in case.

In this file - ipp-samples-ub.txt - you can see where the potentially dangerous code is located.

Conclusion

Dear IPP and IPP Samples developers, we're waiting for your letters. We are ready to discuss and implement missing functionality in the PVS-Studio tool that prevents you from using it. We are also ready to implement diagnostic rules relevant to your project.

And all the rest I wish bugless code and invite to our twitter @Code_Analysis where we post links to interesting articles on C++, programming, static code analysis and the PVS-Studio tool.