Our website uses cookies to enhance your browsing experience.
Accept
to the top
close form

Fill out the form in 2 simple steps below:

Your contact information:

Step 1
Congratulations! This is your promo code!

Desired license type:

Step 2
Team license
Enterprise license
** By clicking this button you agree to our Privacy Policy statement
close form
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
* By clicking this button you agree to our Privacy Policy statement

close form
Free PVS‑Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

close form
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

close form
I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

close form
check circle
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.

>
>
>
PVS-Studio analyzer scans Snort, networ…

PVS-Studio analyzer scans Snort, network traffic scanner

Mar 17 2021

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

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 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 and here.

Generating a report with analysis results

Use the make command to build the Snort project. This short tutorial 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:

pvs-studio-analyzer trace – make

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

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);
  • 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 utility to create a FullHtml 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. 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:

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:

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

#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:

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

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

((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

#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:

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

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.

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

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

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

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

#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

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:

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

A different version is also possible:

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 and see for yourself.

Warning #8 - Perpetual motion machine

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

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

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:

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

Or the following:

!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

#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:

#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:

#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

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

#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:

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

#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

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

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

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

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

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

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

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.

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 when developing, and you'll save some of your time and brain cells.

Popular related articles


Comments (0)

Next comments next comments
close comment form