Various fixes for CodeChecker warnings by craftmaster1231 · Pull Request #8938 · FirebirdSQL/firebird

craftmaster1231 · 2026-03-11T08:21:53Z

This PR contains warning fixes contributed by several students.
Each student was assigned a specific set of warnings to resolve.
The warnings were generated using CodeChecker tool on a Release build.
Most fixes are self-explanatory.

See:
https://codechecker.readthedocs.io/en/latest/

The 'can_use_more' variable is now initialized with a default value at declaration. This resolves the "variable may be uninitialized" compiler warning that occurred when the initial 'length' was zero.

The 'long_value' variable was declared at the start of a loop without a default value. This created a risk of being used with a garbage value, triggering compiler warnings.

This prevents potential use of an uninitialized variable, silences static analysis warnings, and makes the code safer and more predictable, even if the problematic code path was not reachable under the current logic.

The 'length' variable was declared without a default value in a function with complex control flow (two separate switch statements).

Initialized variable 'n' in blr_print_verb to 0 to avoid undefined behavior and remove compiler warning.

The 'savNumber' variable was not initialized when declared. This resulted in a compiler warning and risk of using a garbage value in 'case Request::req_unwind' if the transaction was a system one (TRA_system). Added default initialization ({}) to ensure predictable behavior and fix a bug.

The 'parentStream' variable was not initialized when declared. This could lead to a random value being used on the second and subsequent loop iterations in the pass1Erase function. Added default initialization ({}) to ensure safe behavior.

The 'returnsPos' variable was being set in two separate but logically mutually exclusive 'if' blocks. This was causing a false compiler warning about the possible use of an uninitialized variable. Added initialization of 'returnsPos = 0' on declaration to eliminate the warning.

The 'parentStream' and 'parentNewStream' variables were not initialized when declared. This caused the compiler to warn about possible use of their values before assignment on the second and subsequent iterations of the loop. Added default initialization ({}) to both variables to ensure safe behavior.

The 'parentStream' variable was not initialized when declared. This was causing a compiler warning. Added default initialization ({}) to the variable to ensure safe behavior.

The 'value' pointer was not initialized, which created a risk of dereferencing it with a random value in one of the branches of the conditional logic. This resulted in a compiler warning and potential undefined behavior. Added initialization of the pointer to 'nullptr' to ensure a safe state.

In UnicodeUtil::Utf16Collation::stringToKey function, 'lastCharKeyLen' variable was left uninitialized if 'srcLenLong' was zero. This resulted in reading random value from memory and undefined behavior. Added zero initialization for 'prefixLen' and 'lastCharKeyLen' when they are declared to fix the bug and eliminate compiler warning.

The 'merge_pool' pointer was initialized only under the condition 'count > 1'. Although its reading was protected by the same condition, the static analyzer issued a warning about the possible use of an uninitialized variable. Added initialization of the pointer to the value 'nullptr' when declaring.

In CVT_get_double, the 'value' variable was left uninitialized if the 'default' block was executed in the switch statement. This caused the compiler to warn about the possible use of the variable before it was initialized. Added 'value = 0.0' initialization on declaration to ensure safe behavior in all code paths.

The 'ret_code' variable was not initialized when declared in the do-while loop. Added initialization 'ret_code = 0' to make the code more robust.

The 'minlen' variable was left uninitialized if the data type in 'to_desc' was not handled by any of the 'case' in the switch block. This resulted in reading a random value and undefined behavior. Added initialization 'minlen = 0' on declaration to fix the bug.

Other warns

Disregarded value warns

1) Use bit shifts instead of division by 4 2) Replace % 4 with bitwise AND 3) Use bitwise &/| instead of logical &&/|| for branchless code

…HORT, and the constant initialization with the same value has been removed.

Uninitialized variables

Fixed forming reference to null pointer

…ialized Fix variable may be uninitialized

fixed implicit conversion

Added default initialization to avoid garbage values

Fix by initialization

Fix passing arguments

…flicts Sync with master

aafemt · 2026-03-11T08:58:03Z

src/common/StatusArg.cpp

 void StatusVector::ImplStatusVector::assign(const Exception& ex) noexcept
 {
-	clear();
+	ImplStatusVector::clear();


Using of class name inside of the class' methods is completely pointless.

Virtual method calls bypass virtual dispatch during construction and deconstruction.

I propose to keep names in constructors and destructor to make called functions obvious. Names in other methods should be deleted. I agree, they are pointless and can harm logic with future overrides.

Virtual method calls bypass virtual dispatch during construction and deconstruction.

Yes, and this is well-known and established behavior. Perhaps it is even engraved in standard.

But if your code analyzer insists on fixing it, the right fix would be to remove calls to virtual methods from constructor and destructor, leaving comment why these methods should not be called here, and replacing them with calls to ordinary methods.

aafemt · 2026-03-11T09:01:27Z

src/burp/restore.epp

 *
 **************************************/
-	enum trig_t	type;
+	enum trig_t	type = trig_none;


This value is better to be named trig_invalid and a check for valid type was assigned should be added.

aafemt · 2026-03-11T09:03:56Z

src/common/classes/NoThrowTimeStamp.cpp

+	const int y_1 = y - 1;
+	const int yy = y_1 % 100;
+	const int c = y_1 - yy;
+	const int g = yy + (yy >> 2);


Modern compilers are clever enough to reuse pieces of calculation and replace division by 4 with shifts, but division is better readable by human.

aafemt · 2026-03-11T09:09:13Z

src/dsql/StmtNodes.cpp

 {
 	jrd_tra* transaction = request->req_transaction;
-	SavNumber savNumber;
+	SavNumber savNumber = 0;


It is better to move this declaration to the points of real use to make sure that savepoint zero is never used by any mistake.

aafemt · 2026-03-11T09:12:55Z

src/gpre/c_cxx.cpp

 	args.pat_ident1 = blob->blb_len_ident;
 	args.pat_ident2 = blob->blb_buff_ident;
 	args.pat_string1 = global_status_name;
+	args.pat_long1 = NULL;


Here and in other places where variable type is SLONG: assign zero instead of NULL, please.

aafemt · 2026-03-11T09:15:10Z

src/isql/isql.epp

 				// The bug appears in TCS DSQL_DOMAIN_12 and 13
 				//
-				const double value = *var->value.asFloat;
+				const double value = static_cast<double>(*var->value.asFloat);


Assignment of float to double doesn't lose precision. What is really fixed here?

aafemt · 2026-03-11T09:16:13Z

src/jrd/IntlManager.cpp


 				const ConfigFile::Parameter* module = ch->sub->findParameter("intl_module");
-				const ConfigFile::Parameter* objModule;
+				const ConfigFile::Parameter* objModule = NULL;


nullptr instead of NULL, please.

aafemt · 2026-03-11T09:19:33Z

src/jrd/vio.cpp

-	else if (org_rpb->rpb_record) // we apply update to delete stub
+	else // we apply update to delete stub
 	{
+	    fb_assert(org_rpb->rpb_record);


After this change fb_assert below lost purpose and can be removed. The comment may be moved inside and preserved, but, please, erase reference to me from it.

aafemt · 2026-03-11T09:32:27Z

src/jrd/btr.h

 struct temporary_mini_key
 {
-	USHORT key_length;
+	USHORT key_length = 0;


Converting POD type to constructible one should be very well considered. Especially in this ancient and dark piece of code.

aafemt · 2026-03-11T09:55:40Z

src/common/isc_sync.cpp

 #endif

-				memset(sh_mem_mutex->mtx_mutex, 0, sizeof(*(sh_mem_mutex->mtx_mutex)));
+				memset(sh_mem_mutex->mtx_mutex, 0, sizeof(sh_mem_mutex->mtx_mutex));


This change is definitely wrong.

mtx_mutex is:
pthread_mutex_t mtx_mutex[1];

Sizeof will return same value.
After this change memset still nulls out whole array, but without accessing pthread_mutex_t as an object (even in non-evaluated context).

Indeed, I was wrong overlooking the declaration. Whole this memset in this case looks unneeded because of immediately following pthread_mutex_init call.

Here is what said with BUGGY_LINUX_MUTEX macro enabling. This nulling out gets executed both with and without this macro.

#if (defined(HAVE_PTHREAD_MUTEXATTR_SETPROTOCOL) || defined(USE_ROBUST_MUTEX)) && defined(LINUX) // glibc in linux does not conform to the posix standard. When there is no RT kernel, // ENOTSUP is returned not by pthread_mutexattr_setprotocol(), but by // pthread_mutex_init(). Use a hack to deal with this broken error reporting. #define BUGGY_LINUX_MUTEX #endif

Does it matter? Value returned by pthread_mutex_init is handled correctly and cleaning of the variable is the first thing that happen inside after attributes validation.

There are cases ENOTSUP is returned here and here before this cleaning, which is expected here and here as return from pthread_mutex_init with BUGGY_LINUX_MUTEX after firebird's cleaning.

In my understanding, if ENOTSUP is returned, mtx_mutex will not be cleaned after call, so it is easier to do it unconditionally.

…hods Revert "Fix calling virtual methods in constructor"

sim1984 · 2026-03-17T07:42:07Z

In my opinion, such huge PRs without a clear goal have zero chance of being accepted. It's possible to fix some warnings in a separate subsystem or a small number of files so they can be easily reviewed.

AlexPeshkoff · 2026-03-17T08:53:01Z

In my opinion, such huge PRs without a clear goal have zero chance of being accepted. It's possible to fix some warnings in a separate subsystem or a small number of files so they can be easily reviewed.

Agreed. Moreover, it contains errors like was shown by @aafemt . I suggest to reject PR.

aafemt · 2026-03-17T09:05:51Z

Errors can be fixed, that's a normal process of learning for students, but I agree: several PRs aimed to separate issues would be better.

craftmaster1231 · 2026-03-17T09:20:33Z

Errors discovered by @aafemt will be fixed by students. Thank you for review!
If this PR is going to be rejected due to unclear goals and size even after fixes, would it be OK to open 8-12 different PRs grouped by the purpose of the fixes? Changes were already grouped, but I decided not to create PR flood.

AlexPeshkoff · 2026-03-17T10:37:02Z

On 3/17/26 12:20, Alex wrote: *craftmaster1231* left a comment (FirebirdSQL/firebird#8938) <#8938 (comment)> Errors discovered by @aafemt <https://github.com/aafemt> will be fixed by students. Thank you for your review! If this PR is going to be rejected due to unclear goals and size even after fixes, would it be OK to open 8-12 different PRs grouped by the purpose of the fixes? Changes were already grouped, but I decided to not create PR flood.

Please do.

AlexPeshkoff · 2026-03-18T16:51:41Z

Closed in favor of set of smaller PRs.

Zakk0n and others added 30 commits August 27, 2025 20:50

Null pointers done

66bfb4f

Fixed pointer that should not be dereferenced

3ec8851

fix: ensure null termination when converting string_view to string

4608d5b

fix: correcting the passing of arguments to a function and loop

49b01df

fix: initializing a variable of type FB_UINT64 with a null value

fd6857c

initializing a variable of type SINT64 with a null value

a6988a4

changed the .ebb file, but the warning didn't go away

4e97b5c

initializing a variable of type USHORT with a null value

7ec3f4b

initializing a pointer of type SLONG* nullptr

a6db65e

initializing a pointer of type UCHAR* nullptr

0fe3203

initializing variables with zero and default value

533aab5

Fix snprintf return value

bf4e792

Fix unused parameters

96f1dc0

Fix calling virtual methods in constructor

64ef6a7

fix: Initialize can_use_more to prevent compiler warning

e6dd217

The 'can_use_more' variable is now initialized with a default value at declaration. This resolves the "variable may be uninitialized" compiler warning that occurred when the initial 'length' was zero.

fix: Initialize long_value to prevent potential errors

0e585d9

The 'long_value' variable was declared at the start of a loop without a default value. This created a risk of being used with a garbage value, triggering compiler warnings.

fix: Initialize value variable to improve code robustness

2bebfda

This prevents potential use of an uninitialized variable, silences static analysis warnings, and makes the code safer and more predictable, even if the problematic code path was not reachable under the current logic.

fix: Initialize 'length' variable in blr_print_dtype

c1ed5c5

The 'length' variable was declared without a default value in a function with complex control flow (two separate switch statements).

fix: Fixed warning about uninitialized variable n

80189d0

Initialized variable 'n' in blr_print_verb to 0 to avoid undefined behavior and remove compiler warning.

fix: Initialize parentStream variable in StoreNode::pass1Store

f151bfe

The 'parentStream' variable was not initialized when declared. This was causing a compiler warning. Added default initialization ({}) to the variable to ensure safe behavior.

fix: Initialize ret_code variable in main loop

59c6b11

The 'ret_code' variable was not initialized when declared in the do-while loop. Added initialization 'ret_code = 0' to make the code more robust.

craftmaster1231 and others added 18 commits October 27, 2025 08:58

Merge pull request #23 from sirisospro/other_warns

4e21aab

Other warns

Merge pull request #28 from sirisospro/disregarded_value_warns

f61cff8

Disregarded value warns

refactor: indentation fixed

9464971

refactor: optimize week date conversion with bitwise operations

5a10754

1) Use bit shifts instead of division by 4 2) Replace % 4 with bitwise AND 3) Use bitwise &/| instead of logical &&/|| for branchless code

refactor: the type of the variable len has been changed to const US…

b122e4d

…HORT, and the constant initialization with the same value has been removed.

Merge pull request #18 from laBobberto/uninitialized_variables

a13dca2

Uninitialized variables

Merge pull request #33 from lDeathlWhisperl/fix_forming_nullptr

c284505

Fixed forming reference to null pointer

Merge branch 'master_fix_warns' into fix_variable_may_be_uninitialized

6aca13d

Merge pull request #9 from lDeathlWhisperl/fix_variable_may_be_uninit…

3e66832

…ialized Fix variable may be uninitialized

Merge pull request #12 from lDeathlWhisperl/fix_implicit_conversion

88d1557

fixed implicit conversion

Merge pull request #14 from lDeathlWhisperl/fix_garbage_value

09efd7f

Added default initialization to avoid garbage values

fixed implicit type conversion errors

f43dd1a

the default value for trig_t has been added and initialized

bfa093b

the default value is 0 as there will be a recalculation

f335f7c

Merge pull request #4 from 3qupo/fix_by_initialization

0989f39

Fix by initialization

Merge pull request #5 from 3qupo/fix_passing_arguments

e89decc

Fix passing arguments

Merge branch 'master_fix_warns' into master_fix_branch_fix_conflicts

e8a6aee

Merge pull request #45 from craftmaster1231/master_fix_branch_fix_con…

fe60534

…flicts Sync with master

aafemt reviewed Mar 11, 2026

View reviewed changes

craftmaster1231 added 2 commits March 17, 2026 10:31

Revert "Fix calling virtual methods in constructor"

c35b939

Merge pull request #46 from craftmaster1231/revert-31-fix_virtual_met…

fb86722

…hods Revert "Fix calling virtual methods in constructor"

AlexPeshkoff closed this Mar 18, 2026

Uh oh!

Conversation

craftmaster1231 commented Mar 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craftmaster1231 Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craftmaster1231 Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sim1984 commented Mar 17, 2026

Uh oh!

AlexPeshkoff commented Mar 17, 2026

Uh oh!

aafemt commented Mar 17, 2026

Uh oh!

craftmaster1231 commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexPeshkoff commented Mar 17, 2026 via email

Uh oh!

AlexPeshkoff commented Mar 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

craftmaster1231 Mar 17, 2026 •

edited

Loading

craftmaster1231 Mar 17, 2026 •

edited

Loading

craftmaster1231 commented Mar 17, 2026 •

edited

Loading