To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Checking MatrixSSL with PVS-Studio and …

Checking MatrixSSL with PVS-Studio and Cppcheck

Feb. 2, 2015
Author:

In this article, I'm going to tell you about a check of the MatrixSSL project done with the static analyzers for C/C++ code PVS-Studio and Cppcheck.

0304_MatrixSSL_PVS-Studio/image1.png

The article is written by Pavel Pimenov, the author of the open peer-to-peer client FlylinkDC++. The article is published in our blog by his permission.

What I liked about the MatrixSSL project was that it came with the MS Visual Studio 2010 version available "out-of-the-box".

You know, in order to be able to build openSSL from source files for Visual C++, you usually have to dance around with a shaman's drum for a while :). That's why many Windows developers use ready binary openSSL builds such as Win32 OpenSSL Installation Project.

MatrixSSL is an alternative library of cryptographic algorithms distributed under the GNU license (commercial support is also available).

The source code of the open-source version can be downloaded from the official site. We analyzed the current version 3.7.1.

About the analyzers

  • PVS-Studio is a commercial static analyzer detecting errors in source code of C/C++/C++11 applications (we used version PVS-Studio 5.21).
  • Cppcheck is a free open-source analyzer (we used version Cppcheck 1.68).

Analysis results by PVS-Studio

Memory clearing

V512 A call of the 'memset' function will lead to underflow of the buffer 'ctx->pad'. hmac.c 136, 222, 356

...
// crypto\digest\digest.h
typedef struct {
#ifdef USE_SHA384
  unsigned char  pad[128];
#else
  unsigned char  pad[64];
#endif  

int32 psHmacMd5Final(psHmacContext_t *ctx, unsigned char *hash)
{ 
  memset(ctx->pad, 0x0, 64);
  return MD5_HASH_SIZE;
}
...

The code of all the three functions is alright and only the used part of the array is cleared, but the analyzer warns that the size of the requested buffer - 128 bytes - is probably too large.

I think it's OK here but still it's better to clear either 64 or 128 bytes just for the code to look neat. You can write it, for example, like this:

memset(ctx->pad, 0x0, sizeof(ctx->pad));

V597 The compiler could delete the 'memset' function call, which is used to flush 'tmp' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. aes.c 1139

...
int32 psAesEncrypt(psCipherContext_t *ctx, unsigned char *pt,
           unsigned char *ct, uint32 len)
{
  unsigned char  tmp[MAXBLOCKSIZE];
        .....
  memset(tmp, 0x0, sizeof(tmp));
  return len;
}
...

The optimizer throws away the call of the standard memset() function. I guess it may be critical for a crypto library and is a potential break.

Other similar issues: aes.c 1139, aes.c 1190, aes.c 1191, des3.c 1564, des3.c 1609, des3.c 1610, corelib.c 304, pkcs.c 1625, pkcs.c 1680, pkcs.c 1741

V676 It is incorrect to compare the variable of BOOL type with TRUE. Correct expression is: 'QueryPerformanceFrequency(& hiresFreq) == FALSE'. osdep.c 52, 55

...
#define  PS_TRUE  1
#define  PS_FALSE   0  
int osdepTimeOpen(void)
{
  if (QueryPerformanceFrequency(&hiresFreq) != PS_TRUE) {
    return PS_FAILURE;
  }
  if (QueryPerformanceCounter(&hiresStart) != PS_TRUE) {
    return PS_FAILURE;
  }
...

PS_TRUE is declared as "1". MSDN says the following about the return value of the QueryPerformanceFrequency function: "If the installed hardware supports a high-resolution performance counter, the return value is nonzero" So, a safer way to write it is QueryPerformanceCounter() == PS_FALSE

V547 Expression '(id = ssl->sessionId) == ((void *) 0)' is always false. Pointer 'id = ssl->sessionId' != NULL. matrixssl.c 2061

...
typedef struct ssl {
        ...
  unsigned char  sessionIdLen;
  unsigned char  sessionId[SSL_MAX_SESSION_ID_SIZE];

int32 matrixUpdateSession(ssl_t *ssl)
{
#ifndef USE_PKCS11_TLS_ALGS
  unsigned char  *id;
  uint32  i;

  if (!(ssl->flags & SSL_FLAGS_SERVER)) {
    return PS_ARG_FAIL;
  }
  if ((id = ssl->sessionId) == NULL) {
    return PS_ARG_FAIL;
  }
...

There's an obvious error here: The condition will never be fulfilled because sessionld is declared as an array of 32 bytes and can't have a NULL address. This error is not critical of course and could probably be viewed just as an excessive pointless check.

V560 A part of conditional expression is always true: 0x00000002. osdep.c 265

...
#define FILE_SHARE_READ                 0x00000001  
#define FILE_SHARE_WRITE                0x00000002  

  if ((hFile = CreateFileA(fileName, GENERIC_READ,
      FILE_SHARE_READ && FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
      FILE_ATTRIBUTE_NORMAL, NULL)) == INVALID_HANDLE_VALUE) {
    psTraceStrCore("Unable to open %s\n", (char*)fileName);
        return PS_PLATFORM_FAIL;
...

We have a typo here: Instead of FILE_SHARE_READ | FILE_SHARE_WRITE, the programmer wrote && and got 1 && 2 == 1

which is equivalent to one FILE_SHARE_READ.

Probably incorrect condition

V590 Consider inspecting the '* c != 0 && * c == 1' expression. The expression is excessive or contains a misprint. ssldecode.c 3539

...
    if (*c != 0 && *c == 1) {
#ifdef USE_ZLIB_COMPRESSION
      ssl->inflate.zalloc = NULL;
...

Probable performance drop

V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. x509.c 226

...
  memset(current, 0x0, sizeof(psList_t));
  chFileBuf = (char*)fileBuf;
  while (fileBufLen > 0) {
  if (((start = strstr(chFileBuf, "-----BEGIN")) != NULL) &&
...
      start += strlen("CERTIFICATE-----");
      if (current == NULL) {
...

In this code, inside the while() loop, the analyzer detected a call of the strlen() function for a parameter which doesn't change. Generally it is not optimal but in this particular case since the strlen() function receives a constant known at the compilation stage, the optimizer in the /O2 mode will remove the function call completely and substitute it with the constant's value calculated at the compilation stage.

Analysis results by Cppcheck

This analyzer generated fewer warnings but there were some among them which PVS-Studio had failed to diagnose.

None of them affect the library's work as they all refer to unit-tests in crypto\test.

"Finishing return-shot in the head"

Consecutive return, break, continue, goto or throw statements are unnecessary. The second statement can never be executed, and so should be removed.

...

int32 psSha224Test(void)
{
  runDigestTime(&ctx, HUGE_CHUNKS, SHA224_ALG);
  
     return PS_SUCCESS;
  return PS_SUCCESS;
}
...

This is a copy-paste error. There are two identical lines at the end: return PS_SUCCESS;.

Another typo of this kind can be found in the function psSha384Test(void).

Memory leak

Memory leak: table

This issue is non-critical in this case but it's nice to see that Cppcheck can catch it. The code is inside files and looks as follows (copy-paste):

  • crypto\test\eccperf\eccperf.c
  • crypto\test\rsaperf\rsaperf.c
...
  table = malloc(tsize * sizeof(uint32));  
  if ((sfd = fopen("perfstat.txt", "w")) == NULL) {
    return PS_FAILURE;
  }
...

Resources are better to be requested right before they are really necessary. If you look at the code in those files, you will see that the table is not used at all, that is, the call of the malloc() function as well as the call of the free(table) function at the end are just excessive.

Conclusion

I am a FlylinkDC++ developer and I've been using the PVS-Studio analyzer granted to us as an open-source project for more than two years now. The analyzer more than once helped us find various bugs both in our own code and third-party libraries' code. Thanks to regular checks, FlylinkDC++'s code has become much more stable and safe. And that's wonderful!

Popular related articles
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: 10.22.2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
Free PVS-Studio for those who develops open source projects

Date: 12.22.2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
The Ultimate Question of Programming, Refactoring, and Everything

Date: 04.14.2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
PVS-Studio ROI

Date: 01.30.2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…
PVS-Studio for Java

Date: 01.17.2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
The Last Line Effect

Date: 05.31.2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
Static analysis as part of the development process in Unreal Engine

Date: 06.27.2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Appreciate Static Code Analysis!

Date: 10.16.2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: 11.21.2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: 07.31.2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept