Firebird 3.0 Verification

Not so long ago, a new version of Firebird DBMS was released. The release became one of the largest in the history of the project: the architecture was greatly reworked, support for multi-threading was added, and performance was improved. Such a significant update was the reason for re-checking Firebird using the PVS-Studio static code analyzer.

Introduction

Firebird is a cross-platform free database management system. The project is written in C ++ and runs on Microsoft Windows, Linux, Mac OS X and many Unix-like operating systems. The DBMS is completely free to use and distribute. Learn more about Firebird on the official website .
')
Earlier, Firebird has already been checked by the analyzer. The report on the previous check can be found in the article " Side effect: we check Firebird with PVS-Studio ". The code with GitHub from the master branch was taken for verification. The assembly is described in detail in the corresponding article on the project website. The source files were analyzed in PVS-Studio Standalone version 6.03 using compiler monitoring interception (Compiler Monitoring). This technology allows you to check projects without integration into the assembly system. The resulting report can be viewed both in the Standalone version and in Visual Studio.

Typos

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

PVS-Studio warning : V501 There are identical sub-expressions 'c! =' _ '' Operator. reader.c 1203

The analyzer found that in the logical operation there are two identical subexpressions c! = '_' . In the last condition, a typo was made and the variable c should be compared with another symbol. In other functions, the check with the '$' symbol is used, perhaps you should use it here:

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

Another example of a mistake from inattention:

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

• V601 The string literal is implicitly cast to the bool type. Inspect the second argument. backup.cpp 6113
• V601 The string literal is implicitly cast to the bool type. Inspect the second argument. backup.cpp 6120

The BURP_error function is incorrectly used here. It is declared as follows:

 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 boolean, and the string is the third argument. As a result, the string literal is cast to true. The function call must be rewritten as BURP_error (315, true, "") or BURP_error (315, false, "").

However, there are cases when only a project developer can recognize an error.

 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 warning : V581 The conditional expressions of the 'if' are agreed alongside each other are identical. Check lines: 506, 509. idx.cpp 509

In the code there are two blocks in succession with the same condition. Maybe one of them made a typo, maybe this situation arose because of copying or deleting individual sections: in any case, such code looks strange.

In the following example, consider the situation associated with pointers.

 static void string_to_datetime(....) { .... const char* p = NULL; const USHORT length = CVT_make_string(desc, ttype_ascii, &p, &buffer, sizeof(buffer), err); const char* const end = p + length; .... while (p < end) { if (*p != ' ' && *p != '\t' && p != 0) { CVT_conversion_error(desc, err); return; } ++p; } .... } 

PVS-Studio warning : V713 The pointer has been used in the same logical expression. cvt.cpp 702

In the condition, the variable p is checked for nullptr immediately after dereference. This may mean that there should have been another condition at the inspection site, or that the verification is superfluous.

If you look at the code above, you can find a similar fragment:

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

In order to avoid such an error, for comparison with zero it is better to use the corresponding literals: '\ 0' for the type char, 0 for numbers and nullptr for pointers. If you take this as a rule, you can avoid many of these silly mistakes.

Dangerous use of memcmp

 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 warning : V642 Saving the 'memcmp' function result inside the short type variable is inappropriate. 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 is equal to str2
• > 0 if str1 is greater than str2

Exact values ​​of the function are not guaranteed when the strings are unequal; therefore, saving the result to a variable smaller than int can lead to the loss of significant bits and violation of application logic.

Extra checks

 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 Warning: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 778, 780. jrd.cpp 778

The analyzer found a test of two opposite conditions. Most likely, the second condition here was no longer necessary as a result of changing the first one and it can be removed, but the author should make a decision here.

Another example of strange branching is the following code snippet.

 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 warning : V560 A part of conditional expression is always false :! Field. int_cxx.cpp 217

If field is not a null pointer, then the code will never reach the condition in else if. Either this test is superfluous, or there should be another comparison in its place. It is not known whether this condition contradicts the logic of the application.

In addition, several unnecessary checks were found in logical expressions.

 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 warning : V728 Alert check can be simplified. The '||' operator expressly surrounded by opposite expressions '! xnet_connect_mutex' and 'xnet_connect_mutex'. xnet.cpp 2231

The if check (! Xnet_connect_mutex || (xnet_connect_mutex && ERRNO == ERROR_ALREADY_EXISTS)) can be simplified to if (! Xnet_connect_mutex || ERRNO == ERROR_ALREADY_EXISTS) . This can be easily proved using the truth table.

Dangerous comparison of unsigned variable

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

PVS-Studio warning : 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, since the function returns an unsigned value. The programmer may have checked the value incorrectly. Taking into account similar comparisons in the project, we can assume that the code should look like this:

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

Null pointer dereferencing

 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 warning : V595 The 'lpName' pointer was used before it was verified against nullptr. Check lines: 2814, 2824. isc_sync.cpp 2814

The V595 warning is the most common in projects tested by PVS-Studio, and Firebird is no exception. Total found more than 30 places with this warning.

In this example, the call to strlen (lpName) comes before checking the pointer to nullptr . This will lead to undefined behavior when trying to pass a null pointer to the function. The pointer dereference is hidden here in the strlen call, which makes it difficult to detect an error if you do not use a static analyzer.

Check for nullptr after new

 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 warning : V668 against c pointer allocated allocated allocated allocated... The exception will be generated in the case of memory allocation error. xnet.cpp 2533

The analyzer warns that the new operator cannot return nullptr — you must use a try-catch or new (std :: nothrow) block to check. However, in this example, everything is somewhat more complicated. To allocate memory, use the FB_NEW macro. It is declared in the alloc.h file:

 #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); } 

It is difficult to say whether this particular example is an error or not, as a non-standard allocator is used. But due to the fact that the definition of the operator is set to throw (std :: bad_alloc) , such a check looks suspicious.

Dangerous use of realloc

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

An expression like ptr = realloc (ptr, size) is bad because if realloc returns nullptr, the pointer to memory will be lost. To avoid this, save the realloc result to a temporary variable and, after checking for nullptr, assign ptr to its value.

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

Unused enum values ​​in switch

 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 Warning: V719 The switch statement doesn’t cover all of the PatternItemType enum: piDirectMatch. evl_string.h 324

Not all enum values ​​were used in the switch construction, and there is no default block. It is possible that they forgot to add piDirectMatch processing. List of places with the same warning:

• V719 The switch statement doesn’t cover all of the PatternItemType enum: piDirectMatch, piSkipMore. evl_string.h 351
• V719 The switch statement doesn’t cover all of the 'PatternItemType' enum: piDirectMatch. evl_string.h 368
• V719 The switch statement doesn’t cover all of the 'PatternItemType' enum: piDirectMatch. evl_string.h 387

Buffer overflow

 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 warning : V557 Array overrun is possible. The value of 'prefixLen ++' index could reach 124. restore.cpp 10040

The buffer size isc_844.isc_874 is 125, respectively, the maximum possible value of strlen (isc_844.isc_874) is 124. The temp size is 64, which is less than this value. Writing at this index may result in a buffer overflow. It is safer to allocate more memory for the temp variable.

Shift negative numbers

 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 warning : V610 Unspecified behavior. Check the shift operator '>>'. The left operand is negative ('literal' = [-32768..32767]). array.cpp 848

The code contains a right shift of a negative number. The C ++ standard states that such an action has unspecified behavior, which means it can produce different results on different compilers and platforms. It is better to rewrite it like this:

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

Another place where the warning worked:

V610 Unspecified behavior. Check the shift operator '>>'. The left operand is negative ('i64value' = [-2147483648..2147483647]). exprnodes.cpp 6382

Variable override

 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 warning : V561 It's not better to declare it an exit_code 'variable than to declare it anew. Previous declaration: svc.cpp, line 1893. svc.cpp 1898

In this example, instead of assignment, the variable exit_code is redefined. This hides the previous variable from the scope, and as a result, the function returns an incorrect value, always equal to -1.

Correct code:

 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

Most of the problems found in the previous test were fixed by the developers of the project and now they are not in the code - it means the analyzer has done an excellent job. However, the best result can be achieved with regular use, which will allow you to catch errors at an early stage. Incremental analysis and compatibility with any build systems make it easy to integrate a static analyzer into your project. Using a static analyzer can save a lot of time and find errors that are difficult to detect with debugging or dynamic analysis.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Pavel Belikov. Analyzing Firebird 3.0 .