Side effect: we check Firebird with PVS-Studio

Now we are busy with a big task. We want to compare four code analyzers: Cppcheck, PVS-Studio and Visual Studio 2013 (built-in code analyzer). For this, we decided to check at least 10 open projects and analyze the reports that the analyzers will issue. This is a very time consuming task and until it is completed. But since a number of projects have already been verified, then you can write articles about some of them. What I'm going to do now. First of all, I will describe what was interesting to find with PVS-Studio in Firebird.

Firebird

Firebird (FirebirdSQL) is a compact, cross-platform, free database management system (DBMS) running on Linux, Microsoft Windows and various Unix platforms.

Website: http://www.firebirdsql.org/

Wikipedia description: Firebird
')
Let's see what interesting can be found in the code of this project, using PVS-Studio .

Uninitialized variables

static const UCHAR* compile(const UCHAR* sdl, sdl_arg* arg) { SLONG n, count, variable, value, sdl_operator; .... switch (op) { .... case isc_sdl_add: sdl_operator = op_add; case isc_sdl_subtract: if (!sdl_operator) sdl_operator = op_subtract; ...... } 

V614 Uninitialized variable 'sdl_operator' used. sdl.cpp 404

It seems to me that the 'break' operator between "case isc_sdl_add:" and "case isc_sdl_subtract:" is missing on purpose. It does not take into account the case when we immediately fall into the "case isc_sdl_subtract:". If this happens, the 'sdl_operator' variable will not yet be initialized.

Another similar situation. The variable 'fieldNode' may remain uninitialized if "field == false".

 void blb::move(....) { .... const FieldNode* fieldNode; if (field) { if ((fieldNode = ExprNode::as<FieldNode>(field))) .... } .... const USHORT id = fieldNode->fieldId; .... } 

V614 Potentially uninitialized pointer 'fieldNode' used. blb.cpp 1043

That is why it is bad in a function to give the same name to different variables:

 void realign(....) { for (....) { UCHAR* p = buffer + field->fld_offset; .... for (const burp_fld* field = relation->rel_fields; field; field = field->fld_next) { .... UCHAR* p = buffer + FB_ALIGN(p - buffer, sizeof(SSHORT)); ........ } 

V573 Uninitialized variable 'p' was used. The variable was used to initialize itself. restore.cpp 17535

When initializing the second variable 'p', we wanted to use the value of the first variable 'p'. And it turned out that the second, still uninitialized variable is used.

For the authors of the project. See also here: restore.cpp 17536

Dangerous string comparison (vulnerability)

Please note that the result of the memcmp () function is placed in a variable of the 'SSHORT' type. 'SSHORT' is nothing more than a synonym for the type of 'short'.

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

V642 Saving the 'memcmp' function result inside the 'short' type variable is inappropriate. Breaking the program's logic. texttype.cpp 338

Do you wonder what is wrong here?

Let's remember that the memcmp () function returns a value of type 'int'. But the result is placed in a variable of type 'short'. Loss of high bit values ​​occurs. Is it dangerous!

The function returns the following values: less than zero, zero, or greater than zero. By "greater than zero" can be meant anything. This can be 1, 2 or 19472341. You cannot save the result of the memcmp () function into a variable that is smaller than the size of the 'int' type.

The problem may seem far-fetched. But this is the real problem of vulnerability. This vulnerability was recognized as a similar error in the MySQL code: Security vulnerability in MySQL / MariaDB sql / password.c . There the result was placed in a variable of the type 'char'. The 'short' type is not much better in terms of security.

Similar dangerous comparisons can be found here:

• cvt2.cpp 256
• cvt2.cpp 522

Typos

Typos are everywhere and always. As a rule, most of them are quickly in the process of testing. But still, you can find typos in almost any project.

 int Parser::parseAux() { .... if (yyps->errflag != yyps->errflag) goto yyerrlab; .... } 

V501 There are identical sub-expressions to the left and the right! ”= 'Operator: yyps-> errflag! = Yyps-> errflag parse.cpp 23523

I think comments are not needed. And here, probably, was used Copy-Paste:

 bool CMP_node_match( const qli_nod* node1, const qli_nod* node2) { .... if (node1->nod_desc.dsc_dtype != node2->nod_desc.dsc_dtype || node2->nod_desc.dsc_scale != node2->nod_desc.dsc_scale || node2->nod_desc.dsc_length != node2->nod_desc.dsc_length) .... } 

V501 There are identical sub-expressions "node2-> nod_desc.dsc_scale"! compile.cpp 156

V501 There are identical sub-expressions "node2-> nod_desc.dsc_length"! compile.cpp 157

It turns out that in the CMP_node_match () function the members of the class 'nod_desc.dsc_scale' and 'nod_desc.dsc_length' are incorrectly compared.

Another typo authors of the project can see here: compile.cpp 183

Strange cycles

 static processing_state add_row(TEXT* tabname) { .... unsigned i = n_cols; while (--i >= 0) { if (colnumber[i] == ~0u) { bldr->remove(fbStatus, i); if (ISQL_errmsg(fbStatus)) return (SKIP); } } msg.assignRefNoIncr(bldr->getMetadata(fbStatus)); .... } 

V547 Expression '- i> = 0' is always true. Unsigned type value is always> = 0. isql.cpp 3421

The variable 'i' is of type 'unsigned'. This means that the variable 'i' is always greater than or equal to 0. As a result, the condition (--i> = 0) does not make sense. It is always true.

On the contrary, this cycle will end earlier than necessary:

 SLONG LockManager::queryData(....) { .... for (const srq* lock_srq = (SRQ) SRQ_ABS_PTR(data_header.srq_backward); lock_srq != &data_header; lock_srq = (SRQ) SRQ_ABS_PTR(lock_srq->srq_backward)) { const lbl* const lock = ....; CHECK(lock->lbl_series == series); data = lock->lbl_data; break; } .... } 

What is this suspicious 'break'?

Another similar situation here: pag.cpp 217

Classic

As always, there are a lot of classic flaws associated with pointers. At the beginning, the pointer is dereferenced, and then checked for equality to zero. This may not always lead to an error, but this is a bad and dangerous code. I will show only one example. The remaining places will list in a separate list.

 int CCH_down_grade_dbb(void* ast_object) { .... SyncLockGuard bcbSync( &bcb->bcb_syncObject, SYNC_EXCLUSIVE, "CCH_down_grade_dbb"); bcb->bcb_flags &= ~BCB_exclusive; if (bcb && bcb->bcb_count) .... } 

V595 'bcb ’ Check lines: 271, 274. cch.cpp 271

In the beginning, the 'bcb' pointer is dereferenced in the expression "bcb-> bcb_flags & = ....". The following test shows that 'bcb' may be zero.

List of such places (31 warnings): firebird-V595.txt

Shift operators

Since Firebird is going to be built by different compilers for different platforms, then it is worth correcting the changes that lead to undefined behavior. They may well prove themselves over time in a negative way.

 const ULONG END_BUCKET = (~0) << 1; 

V610 Undefined behavior. Check the shift operator '<<. The left operand '(~ 0)' is negative. ods.h 337

You can not move negative numbers. More: " Without knowing the ford, do not climb into the water. Part three ."

Here it is better to do this:

 const ULONG END_BUCKET = (~0u) << 1; 

And two more bad shifts:

• exprnodes.cpp 6185
• array.cpp 845

Senseless checks

 static processing_state add_row(TEXT* tabname) { .... unsigned varLength, scale; .... scale = msg->getScale(fbStatus, i); .... if (scale < 0) .... } 

V547 Expression 'scale <0' is always false. Unsigned type value is never <0. isql.cpp 3716

The variable 'scale' is of type 'unsigned'. A comparison (scale <0) does not make sense.

Similarly: isql.cpp 4437

Let's look at another function:

 static bool get_switches(....) .... if (**argv != 'n' || **argv != 'N') { fprintf(stderr, "-sqlda : " "Deprecated Feature: you must use XSQLDA\n "); print_switches(); return false; } .... } 

Command line arguments are incorrectly processed. The condition (** argv! = 'N' || ** argv! = 'N') is always satisfied.

miscellanea

 void FB_CARG Why::UtlInterface::getPerfCounters( ...., ISC_INT64* counters) { unsigned n = 0; .... memset(counters, 0, n * sizeof(ISC_INT64)); .... } 

V575 The 'memset' function processes '0' elements. Inspect the third argument. perf.cpp 487

It seems that the variable 'n' in the body of the function forgot to assign a value other than zero.

The convert () function takes the length of the string as the third argument:

 ULONG convert(const ULONG srcLen, const UCHAR* src, const ULONG dstLen, UCHAR* dst, ULONG* badInputPos = NULL, bool ignoreTrailingSpaces = false); 

However, the function is used incorrectly:

 string IntlUtil::escapeAttribute(....) { .... ULONG l; UCHAR* uc = (UCHAR*)(&l); const ULONG uSize = cs->getConvToUnicode().convert(size, p, sizeof(uc), uc); .... } 

V579 It is possibly a mistake. Inspect the third argument. intlutil.cpp 668

We are dealing with a 64-bit error that will manifest itself in Win64.

The expression 'sizeof (uc)' returns the size of the pointer, not the size of the buffer. This is not a problem if the size of the pointer coincides with the size of a variable of type 'unsigned long'. The pointer size is the same as the 'long' size if we are programming for Linux. There will be no problems in Win32.

The error occurs when we compile the application for Win64. The convert () function will think that the buffer size is 8 bytes (pointer size). Actually, the buffer size is 4 bytes.

Note. Perhaps there are other 64-bit errors in the program. But I have not studied the appropriate diagnostics. It's boring to write about them and without knowing the program it is difficult to understand whether there will be an error or not. The described 64-bit problem was found indirectly when studying general-purpose warnings.

Conclusion

The reader is probably interested to know what was found in this project using Cppcheck and VS2013. Yes, these analyzers have found something that PVS-Studio failed. But quite a bit. PVS-Studio has shown itself to be the leader on this project. More information can be found in the comparison article, which we will begin to write soon.