# PVS\-Studio analyzer scans Snort, network traffic scanner

Snort is the most widely used Intrusion Detection System \(IDS\) in the world\. Anyone who's ever dealt with information security is probably familiar with Snort\. Can the PVS\-Studio static analyzer find bugs and potential vulnerabilities in this cool tool? Let's see\!

![0812_Snort/image1.png](https://import.viva64.com/docx/blog/0812_Snort/image1.png)

## Introduction

IDS is an intrusion detection system designed to register suspicious network activity: network attacks against vulnerable services; unauthorized access to important files; attempts to escalate privileges; and virus, Trojan and worm activity\. IDS tools provide an additional shield for computer systems\.

_Snort_ is the most popular free network Intrusion Prevention System \(IPS\) and Intrusion Detection System \(IDS\)\. Snort can register packets and in real time analyzes IP network traffic, blocks, and prevents attacks\. The tool was created by Martin Roesch in 1999 and became so popular that the _Cisco_ network giant acquired it in 2014\. 

Two latest _Snort_ versions are currently available: _Snort_ 2\.9\.17 in C and _Snort_ 3\.1\.1 in C\+\+\. In this article, we'll review the very well known C version of Snort\. We'll write a separate article on the new _Snort_ in C\+\+\. Then we'll contrast and compare both versions to find out whose code is better\.

## PVS\-Studio

The [_PVS\-Studio_](https://pvs-studio.com/en/) tool detects errors and potential vulnerabilities in the source code of programs written in C, C\+\+, C\#, and Java\. It runs on 64\-bit Windows, Linux, and macOS systems and can analyze code designed for 32\-bit, 64\-bit, and embedded ARM platforms\. The most efficient way to use _PVS\-Studio_ is right after compilation\. This way you can find errors before testing the code, thus spending less time debugging\.

_Snort_ 2\.9\.17 in C is written for Linux, so we will use _PVS\-Studio_ for Linux\. To learn how to install and run the analyzer, click [here](https://pvs-studio.com/en/docs/manual/0039/) and [here](https://pvs-studio.com/en/docs/manual/0036/)\. 

## Generating a report with analysis results

Use the _make_ command to build the Snort project\. This [short tutorial](https://pvs-studio.com/en/docs/manual/0036/) explains which commands you need to check this project\. The instructions say that we require the _strace_ utility\. So, what do we need to do?

1\) Run the make command to start the snort build:

```cpp
pvs-studio-analyzer trace – make
```

2\) After the build succeeds, run the following command to start the analysis:

```cpp
pvs-studio-analyzer analyze -l path_to_PVS_Studio.lic \
-a GA;OP -o logfile.log -j <N>
```

This is what the command means:

* _path\_to\_PVS\_Studio_\.lic \- a path to the PVS\-Studio license \(you can request a trial key to try PVS\-Studio for free [here](https://pvs-studio.com/en/pvs-studio/download/)\);
* _logfile\.log_ \- a file that contains a fully encoded analysis result;
* _<N\>_ \- a number of processors we'll allocate for analysis;
* \-_a GA;OP_ – diagnostics groups used for analysis \(by default only_ GA_ is used\)\.

Below is a list of all diagnostics groups available at the moment and in the near future:

* GA – General analysis;
* 64 – 64\-bit Analysis;
* OP \- Micro\-optimizations;
* CS \- Customers Specific Requests;
* MISRA – MISRA guidelines;
* AUTOSAR – AUTOSAR guidelines \(expected\);
* OWASP – OWASP guidelines \(expected\)\.

3\) The last step is to convert the analysis result into a convenient report for review\. Use the [_Plog Converter_](https://github.com/viva64/plog-converter)_ _utility to create a [_FullHtml_](https://pvs-studio.com/en/docs/manual/0036/) report\. This report format is convenient, because you can view it on any device\. You can sort warnings by level, diagnostic number, group, and file\. You can open a warning's target file and access the indicated line in one click\. Clicking a diagnostic's number redirects you to the page with the diagnostic's detailed description\. 

Other ways to study the analysis results on Linux are available [here](https://pvs-studio.com/en/docs/manual/0036/)\. You can filter warnings by group and by diagnostic number\.   

To generate a _FullHtml_ report for all _General analysis_ warnings of level _High_ and _Medium_, run the following command:

```cpp
plog-converter -a GA:1,2 -t fullhtml logfile.log \
-o path_to_report_dir
```

This is what the command means: 

* _GA:1,2_ – a set of general diagnostics of levels _High_ and _Medium_, 
* _path\_to\_project_ – a path to a folder that stores the generated report\.



There were quite a lot of _General analysis_ warnings in the report, so in this article I reviewed only those\. To generate a report with _Micro\-optimizations_ warnings, you can run the following command: 

```cpp
plog-converter -a OP:1,2,3 -t fullhtml path_to_project.log \
-o path_to_report_dir
```

Let's get back to our initial report\. Open it in any browser to review the analysis results\.

## Analysis results

**Warning \#1 \- Yes && no equals no**

V560 A part of conditional expression is always false: \!p\-\>tcph\. sp\_rpc\_check\.c 285 

V560 A part of conditional expression is always false: \!p\-\>udph\. sp\_rpc\_check\.c 286

```cpp
#define IsTCP(p) (IsIP(p) && p->tcph)
#define IsUDP(p) (IsIP(p) && p->udph)
int CheckRpc(void *option_data, Packet *p)
{
  ....
  if (!p->iph_api || (IsTCP(p) && !p->tcph)
                  || (IsUDP(p) && !p->udph))
  {
    return 0; /* if error occured while ip header
               * was processed, return 0 automagically.  */
  }
  ....
}
```

A seemingly logical condition loses its meaning after the macro is expanded\. _PVS\-Studio_ tells us that the _\!p\-\>tcph_ expression is always false, but why? Well, if the condition inside the macro is true, then _p\-\>tcph_ does not equal zero\. After we expand the macro, we get the following: 

```cpp
((IsIP(p) && p->tcph) && !p->tcph)
```

This expression is always _false_, because _x && \!x \= 0_\. The code line below contains the same error:

```cpp
((IsIP(p) && p->tcph) && !p->ucph)
```

This is probably not what the author intended to achieve\. Otherwise, the developer would have left only one condition: _if \(\!p\-\>iph\_api\)_\. The function does not check whether the _p_ variable is TCP or UDP, which is why it might not always work correctly\.

**Warning \#2 \- An unsafe macro**

V634 The priority of the '\*' operation is higher than that of the '<<' operation\. It's possible that parentheses should be used in the expression\. bug34427\.c 160

```cpp
#define PM_EXP2(A) 1 << A

int process_val(const u_int8_t *data, u_int32_t data_len,
                               u_int32_t *retvalue, ....) 
{
  *retvalue = 0;
  ....
  /* Now find the actual value */
  for (; i < data_len; i++) {
    *retvalue += data[i] * PM_EXP2(8 * (data_len - i - 1));
  }
  return(0);
}
```

The analyzer warns that after the macro expands, it may produce an incorrect expression_\._ The function will first multiply the variable by one, and then conduct the bitwise shift to the expression in parentheses\. It was a lucky coincidence that in this line the _x \* 1 << y_ expression is equal to _x \* \(1  << y\)_\.  If to its left or right the macro has _/_, _%_, _\+_, _\-_, or other operations with a priority greater than _<<_, or if the macro contains an operation that has a lesser priority than _<<_, the expression will not be calculated correctly\. Always wrap the macro and its arguments in parentheses to avoid problems in the future\. The following is correct:

```cpp
Define PM_EXP2(A) (1 << (A))
```

This same unsafe macro is also successfully used in the _misc\_ber\.c_ file \(line 97\)\.

**Warning \#3 \- A careless compiler**

V597 The compiler could delete the 'memset' function call, which is used to flush 'ThisFmt' object\. The memset\_s\(\) function should be used to erase the private data\. ftpp\_ui\_config\.c 251

```cpp
void ftpp_ui_config_reset_ftp_cmd_format(FTP_PARAM_FMT *ThisFmt)
{
  ....
  memset(ThisFmt, 0, sizeof(FTP_PARAM_FMT));
  free(ThisFmt);
}
```

One of any compiler's key tasks is optimization\. Why write something to a location where it's of no use? The _memset_ function will be deleted while private data might not be deleted\. The analyzer recommends to use _memset\_s_ so that everything works as intended\. The compiler does not touch this function\. You can read how to safely clear private data [here](https://pvs-studio.com/en/blog/posts/cpp/0388/)\.

You can find another instance of this error here: _spo\_log\_tcpdump\.c_ 485

**Warning** **\#4** **\-** **Ambiguity** 

V595 The 'ssd' pointer was utilized before it was verified against nullptr\. Check lines: 900, 910\. dce2\_smb2\.c 900

```cpp
void DCE2_Smb2Process(DCE2_SmbSsnData *ssd)
{
  const SFSnortPacket *p = ssd->sd.wire_pkt;
  ....
  if (ssd && ssd->pdu_state != DCE2_SMB_PDU_STATE__RAW_DATA)
  {
    ....
  }
  ....
}
```

This behavior is quite strange\. At first the author seems confident that the ssd pointer is not null, but then they start to doubt and checks the pointer for null before use\. Note that _ssd_ is never used anywhere between these two lines\. To make the code easy to understand, it is wise to add a check everywhere or not to check _ssd_ at all\.

Snort triggered one more similar warning:

V595 The 'it' pointer was utilized before it was verified against nullptr\. Check lines: 158, 160\. u2spewfoo\.c 158

```cpp
static inline void free_iterator(u2iterator *it) 
{
  if(it->file) fclose(it->file);
  if(it->filename) free(it->filename);
  if(it) free(it);
}
```

The analyzer noticed odd behavior again\. There is a chance the pointer could be pointing to something that got lost as the code was running\. The _it_ pointer should be checked for _nullptr_ at the very beginning\.

The problem of dereferencing a null pointer is popular among C\\C\+\+ developers\. This did not bypass the Snort project\. It triggered 15 more similar warnings\. Some of the cases are quite ambiguous\. A half of the warnings are listed below:

* The 'bm\_variable\_name' pointer was utilized before it was verified against nullptr\. Check lines: 113, 128\. sf\_snort\_plugin\_byte\.c 113
* V595 The 'cursor' pointer was utilized before it was verified against nullptr\. Check lines: 293, 302\. sf\_snort\_plugin\_pcre\.c 293
* V595 The 'configNext' pointer was utilized before it was verified against nullptr\. Check lines: 782, 788\. spp\_imap\.c 782
* V595 The 'sub\-\>entries' pointer was utilized before it was verified against nullptr\. Check lines: 193, 197\. sfrt\_dir\.c 193
* V595 The 'sub\-\>lengths' pointer was utilized before it was verified against nullptr\. Check lines: 191, 207\. sfrt\_dir\.c 191
* The 'configNext' pointer was utilized before it was verified against nullptr\. Check lines: 778, 784\. spp\_pop\.c 778
* V595 The 'configNext' pointer was utilized before it was verified against nullptr\. Check lines: 809, 816\. spp\_smtp\.c 809
* V595 The 'pmd' pointer was utilized before it was verified against nullptr\. Check lines: 1754, 1761\. fpcreate\.c 1754

**Warning \#5 \- Clear the void**

V575 The null pointer is passed into 'free' function\. Inspect the first argument\. sdf\_us\_ssn\.c 202

```cpp
int ParseSSNGroups(....)
{
  FILE *ssn_file;
  char *contents;
  ....
  contents = (char *)malloc(length + 1);
  if (contents == NULL)
  {
    _dpd.logMsg("Sensitive Data preprocessor: Failed to allocate memory "
      "for SSN groups.\n");

    fclose(ssn_file);
    free(contents); // <=
    return -1;
  }
  ....
  free(contents);
  return 0;
}
```

In this context, zero is always passed to the _free_ function\. This means the function does nothing\. The compiler leaves this action out during optimization\. The developer could have intended to free a different memory portion or could have forgotten to delete this _free_ function call\.

**Warning \#6 \- Failed to share one spot**

V519 The 'port\_array\[5061 / 8\]' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 327, 328\. sip\_config\.c 328

```cpp
#define PORT_INDEX(port) port / 8
#define SIP_PORT 5060
#define SIPS_PORT 5061

static void SIP_ParsePortList(char **ptr, uint8_t *port_array)
{
  ....
  /* If the user specified ports, remove SIP_PORT for now since
   * it now needs to be set explicitly. */
  port_array[PORT_INDEX(SIP_PORT)] = 0;
  port_array[PORT_INDEX(SIPS_PORT)] = 0;
  ....
}
```

The analyzer writes a value to the same location twice\. This is a reason to review the code\. If you expand the macro, you can see that two different ports share same memory cell\. This code needs attention\. You can remove one of the zero assignments or use a different macro altogether\.

**Warning \#7 \- Out of place**

V713 The pointer 'fileEntry\-\>context' was utilized in the logical expression before it was verified against nullptr in the same logical expression\. file\_segment\_process\.c 393

```cpp
static inline int _process_one_file_segment(void* p, 
                          FileEntry *fileEntry, ....)
{
  ....
    if ((fileEntry->context->file_state.sig_state == FILE_SIG_FLUSH)
      && fileEntry->context 
      && fileEntry->context->sha256)
    {
      free(fileEntry->context->sha256);
      fileEntry->context->sha256 = NULL;
    }
  ....
}
```

The pointer is first dereferenced and then checked for _nullptr_ – all in the same conditional expression\. This is a serious typo that will crash the program\. The developer could have been tired and thus inadvertently inserted an additional condition at the very beginning instead of the middle or the end\. Below is the corrected code: 

```cpp
if ( fileEntry->context 
  && fileEntry->context->file_state.sig_state == FILE_SIG_FLUSH
  && fileEntry->context->sha256)
```

A different version is also possible:

```cpp
if ((fileEntry->context->file_state.sig_state == FILE_SIG_FLUSH)
  && fileEntry->context->something 
  && fileEntry->context->sha256
```

Computer programs do not get tired\. Static analyzers always look through every code section with equal scrutiny and warn about buggy or odd code\. Try [_PVS\-Studio_](https://pvs-studio.com/en/pvs-studio/download/) and see for yourself\.

**Warning \#8 \- Perpetual motion machine**

V654 The condition '\!done' of loop is always true\. log\.c 207

```cpp
void PrintNetData(....)
{
  int done;           /* flag */
  ....

  /* initialization */
  done = 0;
  ....

  /* loop thru the whole buffer */
  while(!done)
    {
      ....
    }
  ....
}
```

One would expect an exit from the loop somewhere, but there's none\. The _done_ variable never changes inside the loop, thus creating an infinite loop\. The code snippet above shows all locations with this variable\. There are no pointers or references to this variable\. Once the execution flow reaches the loop, the program will freeze\.

**Warning \#9 \- Check twice\!**

V501 There are identical sub\-expressions '\!info\-\>sip\.\_\_in6\_u\.\_\_u6\_addr32\[0\]' to the left and to the right of the '&&' operator\. pkt\_tracer\.c 160

V501 There are identical sub\-expressions '\!info\-\>dip\.\_\_in6\_u\.\_\_u6\_addr32\[0\]' to the left and to the right of the '&&' operator\. pkt\_tracer\.c 167

```cpp
static inline void debugParse(...., DebugSessionConstraints *info)
{
  ....
  if (!info->sip.s6_addr32[0] && !info->sip.s6_addr32[0] &&
      !info->sip.s6_addr16[4] && info->sip.s6_addr16[5] == 0xFFFF)
  {
    saf = AF_INET;
  }
  else
    saf = AF_INET6;  
  if (!info->dip.s6_addr32[0] && !info->dip.s6_addr32[0] &&
      !info->dip.s6_addr16[4] && info->dip.s6_addr16[5] == 0xFFFF)
  {
    daf = AF_INET;
  }
  else
    daf = AF_INET6;
  ....
}
```

The _\!info\-\>sip\.s6\_addr32\[0\]_ double condition is checked twice in the same function\. This does not help the function work better, but it may cause the function to miss an important condition\. Most likely, the developer missed a typo in one conditional expression and copied it to the second expression\. The correct code could be the following: 

```cpp
!info->sip.s6_addr32[0] && !info->sip.s6_addr32[1]
```

Or the following: 

```cpp
!info->sip.s6_addr32[0] && !info->sip.s6_addr16[0]
```

Or something else\. It's a good idea to review this code\. This function might not work as intended\. 

The analyzer found the exact same code snippet, with the same warnings in the _fw\_appid\.c_ file: 

* V501\. There are identical sub\-expressions '\!info\-\>sip\.\_\_in6\_u\.\_\_u6\_addr32\[0\]' to the left and to the right of the '&&' operator\. fw\_appid\.c 864
* V501 There are identical sub\-expressions '\!info\-\>dip\.\_\_in6\_u\.\_\_u6\_addr32\[0\]' to the left and to the right of the '&&' operator\. fw\_appid\.c 871

**Warning \#10 \- Closed forever**

V621 Consider inspecting the 'for' operator\. It's possible that the loop will be executed incorrectly or won't be executed at all\. snort\_stream\_tcp\.c 2316

V654 The condition 'i < 0' of loop is always false\. snort\_stream\_tcp\.c 2316

```cpp
#define DEFAULT_PORTS_SIZE 0

static void StreamParseTcpArgs(....)
{
  int i;
  ....
    for (i = 0; i < DEFAULT_PORTS_SIZE; i++)
    {
      ....
    }
  ....
}
```

This code fragment triggers two diagnostics at once\. In the release version, the _DEFAULT\_PORTS\_SIZE_ macro is expanded to zero, which is why this for loop will never be executed\. The developer could have planned to use a different macro, or written this cycle for debugging and failed to delete it later\.

**Warning \#11  \- A memory leak**

First, let's take a look at two macros: _BNFA\_MALLOC_ and _BNFA\_FREE\._

The _BNFA\_MALLOC_ macro is expanded as follows: 

```cpp
#define BNFA_MALLOC(n,memory) bnfa_alloc(n,&(memory))
static void * bnfa_alloc( int n, int * m )
{
   void * p = calloc(1,n);
   if( p )
   {
     if(m)
     {
         m[0] += n;
     }
   }
   return p;
}
```

The _BNFA\_FREE_ macro reveals the following:

```cpp
#define BNFA_FREE(p,n,memory) bnfa_free(p,n,&(memory))
static void bnfa_free( void *p, int n, int * m )
{
   if( p )
   {
       free(p);
       if(m)
       {
          m[0] -= n;
       }
   }
}
```

Now let's take a look at _PVS\-Studio_'s warning:

V773 The function was exited without releasing the 'pi' pointer\. A memory leak is possible\. bnfa\_search\.c 1168

```cpp
static
int _bnfa_conv_list_to_csparse_array(bnfa_struct_t * bnfa)
{
  bnfa_state_t    * ps; /* transition list */
  bnfa_state_t    * pi; /* state indexes into ps */
  bnfa_state_t      ps_index = 0;
  unsigned       nps;
  ....

  ps = BNFA_MALLOC(nps*sizeof(bnfa_state_t),
    bnfa->nextstate_memory);
  if (!ps)
  {
    return -1;
  }
  bnfa->bnfaTransList = ps;

  pi = BNFA_MALLOC(bnfa->bnfaNumStates*sizeof(bnfa_state_t),
    bnfa->nextstate_memory); // <=
  if (!pi)
  {
    return -1;
  }
  ....
  if (ps_index > nps)
  {
    return -1; // <=
  }
  ....
  BNFA_FREE(pi,bnfa->bnfaNumStates*sizeof(bnfa_state_t),
    bnfa->nextstate_memory);
  return 0;
}
```

There are two pointers: _ps_ and _pi_\. Only _pi_ triggers the analyzer\. Why? The thing is, the memory area allocated for _ps_, already holds _bnfa\-\>bnfaTransList_, a pointer that is beyond the current function\. This function clears neither _bnfa\-\>bnfaTransList_, nor _ps_ from memory\. This means that the memory is allocated and cleared somewhere else in the program\. The case with _pi_ is completely different\. At the end of the function, _BNFA\_FREE_ clears the memory taken up by _pi_\. However, the memory won't be cleared if the _ps\_index \> nps condition_ is true\. Then the function is not cleared before it exits\. In order for the function to work correctly, copy the function that clears _pi_ and paste it into this conditional operator's body\. 

We encountered a similar situation in a different location: 

V773 The function was exited without releasing the 'ips\_port\_filter\_list' pointer\. A memory leak is possible\. parser\.c 1854

**Warning \#12 \- A meaningless check**

V547 Expression 'rval \!\= \- 6' is always true\. output\_base\.c 219

```cpp
#define OUTPUT_SUCCESS 0
#define OUTPUT_ERROR -1
#define OUTPUT_ERROR_EXISTS -6
static int register_module(....)
{
  ....
  int rval;
  if ((rval = register_plugin(current_dm)) 
                        != OUTPUT_SUCCESS)
    {
      if (rval != OUTPUT_ERROR_EXISTS) // <=
      {
        fprintf(stderr, "%s: Failed to register OUTPUT plugin.\n",
          current_dm->name);
      }
      return OUTPUT_ERROR;
    }
  ....
}
```

Take a look at the _register\_plugin_ function:

```cpp
static int register_plugin(const Output_Module_t *dm)
{
  if (....)
  {
    ....
    return OUTPUT_ERROR;
  }
  ....
  return OUTPUT_SUCCESS;
}
```

The analyzer can see that _rval_ accepts the function's result, and the function returns either _0_, or _\-1_\. Thus, _rval_ cannot be equal to _\-6_\. The _if \(rval \!\= OUTPUT\_ERROR\_EXISTS\)_ condition does not make sense\. _rval_ has a guaranteed value of _\-1_\. It's a good idea to review this code\. The developer may need to use a different variable or fix a typo in the _register\_plugin_ function\.

The analyzer found a similar case in another location:

V547 Expression 'ret \=\= \- 2' is always false\. base\.c 344

```cpp
#define OUTPUT_SUCCESS          0
#define OUTPUT_ERROR           -1
#define OUTPUT_ERROR_NOMEM     -2
#define OUTPUT_ERROR_INVAL     -5

int output_load(const char *directory)
{
  ....
  ret = output_load_module(dirpath);
  if (ret == OUTPUT_SUCCESS)
  {
    DEBUG_WRAP(DebugMessage(DEBUG_INIT, 
      "Found module %s\n", de->d_name););
  }
  else if (ret == OUTPUT_ERROR_NOMEM) // <=
  {
    closedir(dirp);
    return OUTPUT_ERROR_NOMEM;
  }
  ....
}
```

The _output\_load\_module_ function returns one of the following values: _\-5_,_ \-1_, _0_\. This means that the _ret \=\= \-2_ condition is always false\. The developer may need to review the condition or the function\. A typo is possible\.

Here _High_ level warnings end\. This level includes the most important warnings\. They often point to errors that require immediate fixing\. The _Medium_ level's warnings are not as urgent\. However, it is still a good idea for developers to take a look at them\. Let's inspect errors Medium diagnostics found\.

**Warning \#13 \- Macro packaging**

V1004 The 'ppm\_pt' pointer wras used unsafely after it was verified against nullptr\. Check lines: 361, 362\. detect\.c 362

```cpp
ppm_pkt_timer_t  *ppm_pt = NULL;

int Preprocess(Packet * p)
{
  ....
  if( PPM_PKTS_ENABLED() )
  {
    PPM_GET_TIME();
    PPM_TOTAL_PKT_TIME();
    PPM_ACCUM_PKT_TIME();
    ....
  }
  ....
}

#define PPM_TOTAL_PKT_TIME() \
    if( ppm_pt) \
{ \
    ppm_pt->tot = \
      ppm_cur_time - ppm_pt->start - ppm_pt->subtract; \
}

#define PPM_ACCUM_PKT_TIME() \
snort_conf->ppm_cfg.tot_pkt_time += ppm_pt->tot;
```

The _Preprocess_ function almost completely consists of macros that wrap program execution instructions\. This compromises code _readability_\. The developers are likely to get confused, miss something and make a mistake\. And that's exactly what happened\. Next to each other are two macros that perform certain procedures\. When you expand the macros, you can see that while in the first case _ppm\_pt_ is checked for nullptr, in the second case it's not\. This code makes no logical sense\. If _ppm\_pt_ equals to zero, the program will crash\.

**Warning \#14 \- Code for debugging**

V547 Expression 'found\_offset' is always true\. sf\_snort\_plugin\_pcre\.c 202

```cpp
static int pcre_test(...., int *found_offset)
{
  ....
  *found_offset = -1;
  ....

  if (found_offset)
  {
    *found_offset = ovector[1];
    DEBUG_WRAP(DebugMessage(DEBUG_PATTERN_MATCH,
                            "Setting buffer and found_offset: %p %d\n",
                            buf, found_offset););
  }
  return matched;
}
```

This check does not make sense\. If a value was written to the pointer address, the pointer is not null\. If it is not null, the value is rewritten\. The _\*found\_offset \= \-1_ line is likely redundant\. Someone must have added it while debugging\. If _found\_offset_ is null, the program will crash\.

In a different place, the analyzer found the following problem:

V547 Expression 'sipMsg\-\>status\_code \> 0' is always true\. sip\_dialog\.c 806

```cpp
int SIP_updateDialog(SIPMsg *sipMsg,
                     SIP_DialogList *dList,
                     SFSnortPacket *p      )
{
  int ret;
  ....
  if (sipMsg->status_code == 0)
    {
    ret = SIP_processRequest(....);
    }
  else if (sipMsg->status_code > 0)
    {
    ret = SIP_processResponse(....);
    }
  else
    {
    ret = SIP_FAILURE;
    }
  ....
}
```

It's all well and good, but _sipMsg\-\>status\_code_ has the _uint16\_t_ type\. If this element of the _SIPMsg_ structure does not equal zero, it can only be greater than zero\. The first _else_ condition is redundant\. The second _else_ operator's code block is unreachable\. There's no error here, just excessive code\. It's a good idea to avoid it so that developers save time while studying or reworking the code\.

The analyzer found a similar warning in 32 more spots\.

**Warning \#15 \- A redundancy or a typo?**

V560 A part of conditional expression is always true: hnode\. spp\_frag3\.c 4366

```cpp
static int Frag3Prune(FragTracker *not_me)
{
  SFXHASH_NODE *hnode;
  ....
  while (....)
  {
    hnode = sfxhash_lru_node(f_cache);
    if (!hnode)
    {
      break;
    }

    if (hnode && hnode->data == not_me)  // <=
  }
  ....
}
```

There is no need to check _hnode_ for a null pointer here\. If _hnode_ is null, the condition will be skipped anyway\. Or could this be a typo and someone intended to check an \*_hnode_ object's field?

We found a similar warning in 39 more locations\.

**Warning \#16 \- A redundant condition**

V581 The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 300, 308\. sf\_snort\_plugin\_pcre\.c 308

```cpp
static int pcreMatchInternal(...., const uint8_t **cursor)
{
  const uint8_t *buffer_start;
  int pcre_offset;
  int pcre_found;
  ....
  if (pcre_found)
  {
    if (cursor)
    {
      *cursor = buffer_start + pcre_offset;
    }
  }

  if (pcre_found)
    return RULE_MATCH;
  ....
}
```

The code above contains two identical if statements\. Their code blocks perform different actions\. This code is suspicious\. It could be a result of refactoring\. Or it could be a typo that leads to a logical error\. 

**Warning \#17 \- break or return?**

V1001 The 'portsweep' variable is assigned but is not used by the end of the function\. spp\_sfportscan\.c 596

```cpp
static int PortscanAlertTcp(PS_PROTO *proto, ....)
{
  ....
  int portsweep = 0;

  if (!proto)
    return -1;

  switch (proto->alerts)
  {
  case PS_ALERT_ONE_TO_ONE:
    ....
    break;

  case PS_ALERT_ONE_TO_ONE_DECOY:
    ....
    break;

  case PS_ALERT_PORTSWEEP:
    ....
    portsweep = 1;
    break;

  case PS_ALERT_DISTRIBUTED:
    ....
    break;

  case PS_ALERT_ONE_TO_ONE_FILTERED:
    ....
    break;

  case PS_ALERT_ONE_TO_ONE_DECOY_FILTERED:
    ....
    break;

  case PS_ALERT_PORTSWEEP_FILTERED:
    ....
    portsweep = 1; // <=
    return 0;

  case PS_ALERT_DISTRIBUTED_FILTERED:
    ....
    break;

  default:
    return 0;
  }
  ....
}
```

One of the operator's branches assigns a value to a variable, and then the function exits\. This looks odd\.  If you look at other branches, it becomes clear how to fix the code\. One can replace _return_ with _break_ \- or remove the assignment\. 

**Warning \#18 \- When zero is not zero**

V1048 The 'ret' variable was assigned the same value\. sf\_snort\_plugin\_loop\.c 142 

V1048 The 'ret' variable was assigned the same value\. sf\_snort\_plugin\_loop\.c 148

```cpp
int LoopInfoInitialize(...., Rule *rule, LoopInfo *loopInfo)
{
  int ret;

  /* Initialize the dynamic start, end, increment fields */
  ret = DynamicElementInitialize(rule, loopInfo->start);
  if (ret)
  {
    return ret;
  }
  ret = DynamicElementInitialize(rule, loopInfo->end);
  if (ret)
  {
    return ret;
  }
  ret = DynamicElementInitialize(rule, loopInfo->increment);
  if (ret)
  {
    return ret;
  }
  ....
}
```

See the _DynamicElementInitialize_ _function's_ _initialization_ _below\._ Take a look at the returned value\.

```cpp
int DynamicElementInitialize(Rule *rule, DynamicElement *element)
{
  void *memoryLocation;

  if (!rule->ruleData)
  {
    DynamicEngineFatalMessage("ByteExtract variable '%s' "
      "in rule [%d:%d] is used before it is defined.\n", 
      element->refId, rule->info.genID, rule->info.sigID);
  }

  switch (element->dynamicType)
  {
  case DYNAMIC_TYPE_INT_REF:
    memoryLocation = sfghash_find((SFGHASH*)rule->ruleData,
                                           element->refId);
    if (memoryLocation)
    {
       element->data.dynamicInt = memoryLocation;
    }
    else
    {
      element->data.dynamicInt = NULL;
      DynamicEngineFatalMessage("ByteExtract variable '%s' "
        "in rule [%d:%d] is used before it is defined.\n",
        element->refId, rule->info.genID, rule->info.sigID);
      //return -1;
    }
    break;
  case DYNAMIC_TYPE_INT_STATIC:
  default:
    /* nothing to do, its static */
    break;
  }

  return 0;  // <=
}
```

The _DynamicElementInitialize_ function always returns _0_, which is why there is no point in checking the _ret_ value returned by the _LoopInfoInitialize_ function\. There is no point in returning anything at all if only one value can exist\. Earlier the developers may have experimented with _\-1_ \(the commented code attests to this\), but right now that code is of no use\. 

We found a similar warning in 15 more locations\.

The _PVS\-Studio_ analyzer checked the _Snort_ IDS and found 35 potentially unsafe code blocks or errors, as well as 100 code that require review\. They probably do not work as expected\. All in all, the Snort version in C has 470 000 lines \- so this number of errors is not very significant\. The _Snort_ project's developers did a very good job\. They gave a lot of thought when creating their project and made very few mistakes\. However, they could have spent less time debugging and boasted even better\-quality code if they used _PVS\-Studio_\.

In the next article we'll analyze _Snort_ written in C\+\+ and we'll compare the results of the two analyses\. This will demonstrate which error patterns are more common in C apps and which are more typical of C\+\+ programs\. We will also see whether the code became better or whether additional features led to more errors\.

## Conclusion

_PVS\-Studio_ is a convenient and useful tool for developers\. It comes to the rescue and takes the load off the developer in many cases\. When the human brain stops getting multi\-level dependencies in the code\. When the developers lose their attention as a result of fatigue\. When large files are modified and not all the subtleties of the program can be easily noticed in order to add the code correctly\. A static analyzer is a program that will always check code responsibly and attentively\. Use [_PVS\-Studio_](https://pvs-studio.com/en/pvs-studio/) when developing, and you'll save some of your time and brain cells\.