# Analyzing Firebird 3\.0

A new version of Firebird DBMS was released not so long ago\. This release was one of the most significant in the project's history, as it marked substantial revision of the architecture, addition of multithreading support, and performance improvements\. Such a significant update was a good occasion for us to scan Firebird one more time with PVS\-Studio static code analyzer\. 

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

## Introduction

Firebird is a cross\-platform open\-source database management system written in C\+\+ that runs on Microsoft Windows, Linux, macOS X, and many Unix\-like operating systems\. It can be used and distributed for free\. To learn more about Firebird, welcome to the [official site](http://www.firebirdsql.org/)\.

We have already scanned Firebird with our analyzer before\. The previous report can be found in the article "[A Spin\-off: Firebird Checked by PVS\-Studio](https://pvs-studio.com/en/blog/posts/cpp/0235/)"\. For this analysis, we took the project code from [GitHub](https://github.com/firebirdsql/firebird), the master branch\. The building process is described in detail in the [article](http://www.firebirdsql.org/en/building-the-code-windows/) at the project website\. We analyzed the source files in [PVS\-Studio Standalone](https://pvs-studio.com/en/docs/manual/0033/), version 6\.03, using the Compiler Monitoring mechanism, which allows you to scan projects without integrating the tool into the build system\. The log file generated by the analyzer can be viewed both in the Standalone version and in Visual Studio\. 

## Typos

```cpp
void advance_to_start()
{
  ....
  if (!isalpha(c) && c != '_' && c != '.' && c != '_')
    syntax_error(lineno, line, cptr);
  ....
}
```

PVS\-Studio diagnostic message: [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'c \!\= '\_'' to the left and to the right of the '&&' operator\. reader\.c 1203

The analyzer detected a logical expression with two identical subexpressions _c_ _\!\=_ _'\_'_\. The last condition contains a typo and should actually compare the _c_ variable with some other character\. In other functions nearby, the variable is tested for the '$' character, so it should probably be used in our example as well:

```cpp
if (!isalpha(c) && c != '_' && c != '.' && c != '$')
```

Another mistake resulting from the programmer's inattention:

```cpp
int put_message(....)
{
  if (newlen <= MAX_UCHAR)
    {
    put(tdgbl, attribute);
    put(tdgbl, (UCHAR) newlen);
  }
  else if (newlen <= MAX_USHORT)
  {
    if (!attribute2)
      BURP_error(314, "");
    ....
  }
  else
    BURP_error(315, "");
  ....
}
```

PVS\-Studio diagnostic messages:

* [V601](https://pvs-studio.com/en/docs/warnings/v601/) The string literal is implicitly cast to the bool type\. Inspect the second argument\. backup\.cpp 6113
* [V601](https://pvs-studio.com/en/docs/warnings/v601/) The string literal is implicitly cast to the bool type\. Inspect the second argument\. backup\.cpp 6120

Here we deal with a wrong call to the _BURP\_error_ function\. This is how the function is declared: 

```cpp
void BURP_error(USHORT errcode, bool abort,
     const MsgFormat::SafeArg& arg = MsgFormat::SafeArg());

void BURP_error(USHORT errcode, bool abort, const char* str);
```

The second argument is a boolean value and the third one is a string\. In our example, however, the string literal is passed as the second argument and is, therefore, cast to _true\._ The function call should be rewritten in the following way: BURP\_error\(315, true, ""\) or BURP\_error\(315, false, ""\)\.

However, there are cases when only the project authors can tell if there is an error or not\.

```cpp
void IDX_create_index(....)
{
  ....
  index_fast_load ifl_data;
  ....
  if (!ifl_data.ifl_duplicates)
    scb->sort(tdbb);

  if (!ifl_data.ifl_duplicates)
    BTR_create(tdbb, creation, selectivity);

  ....
}
```

PVS\-Studio diagnostic message: [V581](https://pvs-studio.com/en/docs/warnings/v581/) The conditional expressions of the 'if' operators situated alongside each other are identical\. Check lines: 506, 509\. idx\.cpp 509

This example deals with two blocks of code that check the same condition in succession\. There might be a typo in one of them, or this issue has to do with copying or deleting some code fragments\. In any case, this code looks strange\.

In the next example we'll discuss an issue that deals with pointers\.

```cpp
static void string_to_datetime(....)
{
  ....

  const char* p = NULL;
  const char* const end = p + length;

  ....

  while (p < end)
  {
    if (*p != ' ' && *p != '\t' && p != 0)
    {
      CVT_conversion_error(desc, err);
      return;
    }
    ++p;
  }

  ....
}
```

PVS\-Studio diagnostic message: [V713](https://pvs-studio.com/en/docs/warnings/v713/) The pointer p was utilized in the logical expression before it was verified against nullptr in the same logical expression\. cvt\.cpp 702

In the condition, the _p _variable is compared with _nullptr_ right after dereferencing\. It may indicate that some other condition should have been used instead of this check, or that this check is just not necessary\.

Earlier in the code, a similar fragment can be found:

```cpp
while (++p < end)
{
  if (*p != ' ' && *p != '\t' && *p != 0)
    CVT_conversion_error(desc, err);
}
```

To avoid errors like this, use appropriate literals when comparing with zero: _'\\0' _for type _char_, 0 for numbers, and _nullptr_ for pointers\. Sticking to this rule will help you avoid lots of silly errors like that\.

## Unsafe use of memcmp

```cpp
SSHORT TextType::compare(ULONG len1, const UCHAR* str1,
                         ULONG len2, const UCHAR* str2)
{
  ....

  SSHORT cmp = memcmp(str1, str2, MIN(len1, len2));

  if (cmp == 0)
    cmp = (len1 < len2 ? -1 : (len1 > len2 ? 1 : 0));

  return cmp;
}
```

PVS\-Studio diagnostic message: [V642](https://pvs-studio.com/en/docs/warnings/v642/) Saving the 'memcmp' function result inside the 'short' type variable is inappropriate\. The significant bits could be lost breaking the program's logic\. texttype\.cpp 3

The _memcmp_ function returns the following values:

* < 0 if _str1 _is less than_ str2_
* 0 if _str1_ equals _str2_
* \> 0 if _str1_ is greater than _str2_

The function does not guarantee to return exact values when the strings are not equal, so storing the result in a variable of size less than that of type _int_ may lead to losing the most significant bits and distorting the execution logic\.

## Extra checks

```cpp
void Trigger::compile(thread_db* tdbb)
{
  SET_TDBB(tdbb);

  Database* dbb = tdbb->getDatabase();
  Jrd::Attachment* const att = tdbb->getAttachment();

  if (extTrigger)
    return;

  if (!statement /*&& !compile_in_progress*/)
  {
    if (statement)
      return;

    ....
  }
}
```

PVS\-Studio diagnostic message: [V637](https://pvs-studio.com/en/docs/warnings/v637/) Two opposite conditions were encountered\. The second condition is always false\. Check lines: 778, 780\. jrd\.cpp 778

The analyzer detected checks of two opposite conditions\. The second condition seems to be no longer necessary since the first one was changed at some point in the past, so it can be deleted, although it is completely up to the author to make this decision\.

The following code fragment is another example of strange branching\.

```cpp
static void asgn_from( ref* reference, int column)
{
  TEXT variable[MAX_REF_SIZE];
  TEXT temp[MAX_REF_SIZE];

  for (; reference; reference = reference->ref_next)
  {
    const gpre_fld* field = reference->ref_field;
    ....

    if (!field || field->fld_dtype == dtype_text)
      ....
    else if (!field || field->fld_dtype == dtype_cstring)
      ....
    else
      ....
  }
}
```

PVS\-Studio diagnostic message: [V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always false: \!field\. int\_cxx\.cpp 217

If the _field_ pointer is non\-null, the code will never reach the condition in the _else_ _if_ branch\. Either this check is redundant or there should be some other comparison instead of it\. It's not clear, whether this condition contradicts the execution logic\.

In addition to these examples, a number of redundant checks were found in logical expressions\.

```cpp
bool XnetServerEndPoint::server_init(USHORT flag)
{
  ....

  xnet_connect_mutex = CreateMutex(ISC_get_security_desc(),
                          FALSE, name_buffer);
  if (!xnet_connect_mutex ||
          (xnet_connect_mutex && ERRNO == ERROR_ALREADY_EXISTS))
  {
    system_error::raise(ERR_STR("CreateMutex"));
  }

  ....
}
```

PVS\-Studio diagnostic message: [V728](https://pvs-studio.com/en/docs/warnings/v728/) An excessive check can be simplified\. The '\|\|' operator is surrounded by opposite expressions '\!xnet\_connect\_mutex' and 'xnet\_connect\_mutex'\.  xnet\.cpp 2231

The check _if \(\!xnet\_connect\_mutex \|\| \(xnet\_connect\_mutex && ERRNO \=\= ERROR\_ALREADY\_EXISTS\)\)_ can be simplified to _if \(\!xnet\_connect\_mutex \|\| ERRNO \=\= ERROR\_ALREADY\_EXISTS\)_\. The correctness of such transformation can be easily proved with the truth table\.

## Unsafe comparison of an unsigned variable

```cpp
static bool write_page(thread_db* tdbb, BufferDesc* bdb, ....)
{
  ....
  if (bdb->bdb_page.getPageNum() >= 0)
  ....
}
```

PVS\-Studio diagnostic message: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'bdb\-\>bdb\_page\.getPageNum\(\) \>\= 0' is always true\. Unsigned type value is always \>\= 0\. cch\.cpp 4827

The _bdb\-\>bdb\_page\.getPageNum\(\) \>\= 0_ condition will always be true, as the function returns an unsigned value\. This error probably has to do with an incorrect check of the value\. Based on other similar comparisons in the project, I think the code should actually look like this:

```cpp
if (bdb->bdb_page.getPageNum() != 0)
```

## Null pointer dereferencing

```cpp
static bool initializeFastMutex(FAST_MUTEX* lpMutex, 
  LPSECURITY_ATTRIBUTES lpAttributes, BOOL bInitialState,
  LPCSTR lpName)
{
  if (pid == 0)
    pid = GetCurrentProcessId();
  
  LPCSTR name = lpName;

  if (strlen(lpName) + strlen(FAST_MUTEX_EVT_NAME) - 2
                                                   >= MAXPATHLEN)
  {
    SetLastError(ERROR_FILENAME_EXCED_RANGE);
    return false;
  }

  setupMutex(lpMutex);

  char sz[MAXPATHLEN]; 
  if (lpName)
  ....
}
```

PVS\-Studio diagnostic message: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'lpName' pointer was utilized before it was verified against nullptr\. Check lines: 2814, 2824\. isc\_sync\.cpp 2814

Warning V595 is the most common among the projects scanned by PVS\-Studio, and Firebird is no exception\. In total, the analyzer found 30 issues triggering this diagnostic\. 

In this example, the call _strlen\(lpName\)_ precedes a pointer check for _nullptr_, thus leading to undefined behavior when trying to pass a null pointer to the function\. The pointer\-dereferencing operation is hidden inside the call to _strlen_, which makes it difficult to find the error without a static analyzer\.

## Testing for nullptr after new

```cpp
rem_port* XnetServerEndPoint::get_server_port(....)
{
  ....
  XCC xcc = FB_NEW struct xcc(this);

  try {

    ....
  }
  catch (const Exception&)
  {
    if (port)
      cleanup_port(port);
    else if (xcc)
      cleanup_comm(xcc);

    throw;
  }

  return port;
}
```

PVS\-Studio diagnostic message: [V668](https://pvs-studio.com/en/docs/warnings/v668/) There is no sense in testing the 'xcc' pointer against null, as the memory was allocated using the 'new' operator\. The exception will be generated in the case of memory allocation error\. xnet\.cpp 2533

The analyzer warns us that the _new _operator cannot return _nullptr _\- one must use a _try\-catch_ block or _new \(std::nothrow\)_\. However, this example is a bit more complicated\. The programmer uses macro _FB\_NEW_ to allocate memory\. This macro is declared in the file alloc\.h:

```cpp
#ifdef USE_SYSTEM_NEW
#define OOM_EXCEPTION std::bad_alloc
#else
#define OOM_EXCEPTION Firebird::BadAlloc
#endif

#define FB_NEW new(__FILE__, __LINE__)

inline void* operator new(size_t s ALLOC_PARAMS)
throw (OOM_EXCEPTION)
{
  return MemoryPool::globalAlloc(s ALLOC_PASS_ARGS);
}
```

I can't say for sure if this particular example is incorrect, as it uses a non\-standard allocator; but the presence of _throw_ _\(std::bad\_alloc\)_ in the operator declaration makes this check quite suspicious\.

## Unsafe use of realloc

```cpp
int mputchar(struct mstring *s, int ch)
{
  if (!s || !s->base) return ch;
  if (s->ptr == s->end) {
    int len = s->end - s->base;
    if ((s->base = realloc(s->base, len+len+TAIL))) {
      s->ptr = s->base + len;
      s->end = s->base + len+len+TAIL; }
    else {
      s->ptr = s->end = 0;
      return ch; } }
  *s->ptr++ = ch;
  return ch;
}
```

PVS\-Studio diagnostic message: [V701](https://pvs-studio.com/en/docs/warnings/v701/) realloc\(\) possible leak: when realloc\(\) fails in allocating memory, original pointer 's\-\>base' is lost\. Consider assigning realloc\(\) to a temporary pointer\. mstring\.c 42

What is bad about expressions of the _ptr \= realloc\(ptr, size\)_ pattern is that the pointer to the memory block will be lost when _realloc _returns _nullptr_\. To avoid it, one needs to save the result returned by _realloc _in a temporary variable and then assign this value to _ptr _after comparing it with _nullptr_\.

```cpp
temp_ptr = realloc(ptr, new_size);
if (temp_ptr == nullptr) {
  //handle exception
} else {
  ptr = temp_ptr;
}
```

## Unused enum values in switch

```cpp
template <typename CharType>
LikeEvaluator<CharType>::LikeEvaluator(....)
{
  ....
  PatternItem *item = patternItems.begin();
  ....
  switch (item->type)
  {
  case piSkipFixed:
  case piSkipMore:
    patternItems.grow(patternItems.getCount() + 1);
    item = patternItems.end() - 1;
    // Note: fall into
    case piNone:
      item->type = piEscapedString;
      item->str.data = const_cast<CharType*>
                        (pattern_str + pattern_pos - 2);
      item->str.length = 1;
      break;
    case piSearch:
      item->type = piEscapedString;
      // Note: fall into
    case piEscapedString:
      item->str.length++;
      break;
  }
  ....
}
```

PVS\-Studio diagnostic message: [V719](https://pvs-studio.com/en/docs/warnings/v719/) The switch statement does not cover all values of the 'PatternItemType' enum: piDirectMatch\. evl\_string\.h 324

Not all _enum_ values were used in the _switch_ statement; the _default_ block is absent, too\. This example seems to lack the code that handles the _piDirectMatch_ element\. Other similar issues:

* V719 The switch statement does not cover all values of the 'PatternItemType' enum: piDirectMatch, piSkipMore\. evl\_string\.h 351
* V719 The switch statement does not cover all values of the 'PatternItemType' enum: piDirectMatch\. evl\_string\.h 368
* V719 The switch statement does not cover all values of the 'PatternItemType' enum: piDirectMatch\. evl\_string\.h 387

## Buffer overflow

```cpp
const int GDS_NAME_LEN = 32;
....
bool get_function(BurpGlobals* tdgbl)
{
  ....
  struct isc_844_struct {
    ....
    short isc_870; /* gds__null_flag */
    ....
    char  isc_874 [125]; /* RDB$PACKAGE_NAME */
    ....
  } isc_844;
 
  att_type attribute;
  TEXT    temp[GDS_NAME_LEN * 2];
  ....
  SSHORT prefixLen = 0;
  if (!/*X.RDB$PACKAGE_NAME.NULL*/
       isc_844.isc_870)
  {
    prefixLen = static_cast<SSHORT>(strlen(/*X.RDB$PACKAGE_NAME*/
                                           isc_844.isc_874));
    memcpy(temp, /*X.RDB$PACKAGE_NAME*/
                 isc_844.isc_874, prefixLen);
    temp[prefixLen++] = '.';
  }
  ....

}
```

PVS\-Studio diagnostic message: [V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The value of 'prefixLen \+\+' index could reach 124\. restore\.cpp 10040

The size of the buffer _isc\_844\.isc\_874_ is 125; therefore, the largest value possible of _strlen\(isc\_844\.isc\_874\)_ is 124\. The size of _temp_ is 64, which is less than that value\. Writing at this index may cause a buffer overflow\. A safer way is to allocate a larger storage for the _temp_ variable\.

## Shifting negative numbers

```cpp
static ISC_STATUS stuff_literal(gen_t* gen, SLONG literal)
{
  ....

  if (literal >= -32768 && literal <= 32767)
    return stuff_args(gen, 3, isc_sdl_short_integer, literal, 
                      literal >> 8);

  ....

}
```

PVS\-Studio diagnostic message: [V610](https://pvs-studio.com/en/docs/warnings/v610/) Unspecified behavior\. Check the shift operator '\>\>'\. The left operand is negative \('literal' \= \[\-32768\.\.32767\]\)\. array\.cpp 848

The code contains a right\-shift operation on a negative number\. As the C\+\+ standard states, such an operation leads to undefined behavior, i\.e\. it may produce different results on different compilers and platforms\. The code should be rewritten as follows:

```cpp
if (literal >= -32768 && literal <= 32767)
  return stuff_args(gen, 3, isc_sdl_short_integer, literal, 
                    (ULONG)literal >> 8);
```

Another fragment triggering this warning: 

[V610](https://pvs-studio.com/en/docs/warnings/v610/) Unspecified behavior\. Check the shift operator '\>\>'\. The left operand is negative \('i64value' \= \[\-2147483648\.\.2147483647\]\)\. exprnodes\.cpp 6382

## Variable redefinition

```cpp
THREAD_ENTRY_DECLARE Service::run(THREAD_ENTRY_PARAM arg)
{
  int exit_code = -1;
  try
  {
    Service* svc = (Service*)arg;
    RefPtr<SvcMutex> ref(svc->svc_existence);
    int exit_code = svc->svc_service_run->serv_thd(svc);

    svc->started();
    svc->svc_sem_full.release();
    svc->finish(SVC_finished);
  }
  catch (const Exception& ex)
  {
    // Not much we can do here
    iscLogException("Exception in Service::run():", ex);
  }

  return (THREAD_ENTRY_RETURN)(IPTR) exit_code;
}
```

PVS\-Studio diagnostic message: [V561](https://pvs-studio.com/en/docs/warnings/v561/) It's probably better to assign value to 'exit\_code' variable than to declare it anew\. Previous declaration: svc\.cpp, line 1893\. svc\.cpp 1898

In this example, the _exit\_code_ variable is redefined instead of being assigned a value\. Variable redefinition hides the previously declared variable from the scope and makes the function always return an incorrect value, which is \-1\.

Fixed code:

```cpp
THREAD_ENTRY_DECLARE Service::run(THREAD_ENTRY_PARAM arg)
{
  int exit_code = -1;
  try
  {
    Service* svc = (Service*)arg;
    RefPtr<SvcMutex> ref(svc->svc_existence);
    exit_code = svc->svc_service_run->serv_thd(svc);

    svc->started();
    svc->svc_sem_full.release();
    svc->finish(SVC_finished);
  }
  catch (const Exception& ex)
  {
    // Not much we can do here
    iscLogException("Exception in Service::run():", ex);
  }

  return (THREAD_ENTRY_RETURN)(IPTR) exit_code;
}
```

## Conclusion

As the new analysis shows, the project developers have fixed most of the issues found during the [previous analysis](https://pvs-studio.com/en/blog/posts/cpp/0235/), so those bugs are no longer there, which is a good sign that the compiler did a good job\. However, using the analyzer regularly could help achieve even better results because that way it allows catching bugs at earlier stages\. Incremental analysis and compatibility with any build system allow integrating the analyzer easily into your project\. Using static analysis helps save plenty of time and catch errors that are difficult to detect by means of debugging or dynamic analysis\.