Fix #14150 (Add unionzeroinit check) #7843

mptre · 2025-09-22T17:50:11Z

This has bitten me before and is definitely a foot gun. GCC 15[1] changed their semantics related to zero initialization of unions; from initializing the complete union (sizeof union) to only zero initializing the first member. If the same first member is not the largest one, the state of the remaining storage is considered undefined and in practice most likely stack garbage.

The unionzeroinit addon can detect such scenarios and emit a warning. It does not cover the designated initializers as I would interpret those as being intentional.

Example output from one of my projects:

x86-decoder.c:294:7: warning: Zero initializing union Evex does not guarantee its complete storage to be zero initialized as its largest member is not declared as the first member. Consider making u32 the first member or favor memset(). [unionzeroinit-unionzeroinit]
 Evex evex = {0};
      ^

[1] https://trofi.github.io/posts/328-c-union-init-and-gcc-15.html

danmar · 2025-09-23T03:00:47Z

nice work. I would like that we add a ticket in trac and point to that in the PR title.

I am spontanously thinking that this could possibly be a internal core checker instead (portability). Are there some situations when you think users will consider these warnings to be false positives, assuming they want to fix portability issues?

This has bitten me before and is definitely a foot gun. GCC 15[1] changed their semantics related to zero initialization of unions; from initializing the complete union (sizeof union) to only zero initializing the first member. If the same first member is not the largest one, the state of the remaining storage is considered undefined and in practice most likely stack garbage. The unionzeroinit addon can detect such scenarios and emit a warning. It does not cover the designated initializers as I would interpret those as being intentional. Example output from one of my projects: ``` x86-decoder.c:294:7: warning: Zero initializing union Evex does not guarantee its complete storage to be zero initialized as its largest member is not declared as the first member. Consider making u32 the first member or favor memset(). [unionzeroinit-unionzeroinit] Evex evex = {0}; ^ ``` [1] https://trofi.github.io/posts/328-c-union-init-and-gcc-15.html

mptre · 2025-09-23T17:55:35Z

nice work. I would like that we add a ticket in trac and point to that in the PR title.

Done

I am spontanously thinking that this could possibly be a internal core checker instead (portability).

Agreed, this more of a proof-of-concept that turned out well IMO.

Are there some situations when you think users will consider these warnings to be false positives, assuming they want to fix portability issues?

Say you have union { uint8_t u8; uint64_t u64 } u = {0} and only operate on u8 would be a false positive.

I changed the severity from warning to portability.

mptre · 2025-09-23T17:58:50Z

addons/unionzeroinit.py

+
+        nested_variables = variable.nameToken.valueType.typeScope.varlist
+
+        # Circumvent what seems to be a bug in which only the last bitfield has


@danmar this smells like a bug. Reproducable using one of the added test cases.

cppcheck --quiet --dump addons/test/unionzeroinit/bitfields.c

Note that only the r3 field has valueType-bits="1" whereas the remaining bitfields has the same attribute set to zero.

Note that only the r3 field has valueType-bits="1" whereas the remaining bitfields has the same attribute set to zero.

if that is true then yes it is a bug!

would you like to write a ticket about it or should I?

No need, fixed in the following commit (as part of this PR).

7344d5f

danmar · 2025-09-28T14:59:01Z

Say you have union { uint8_t u8; uint64_t u64 } u = {0} and only operate on u8 would be a false positive.

yes true but then it would be pretty redundant to use a union I guess.. I have a good feeling about this checker and would like to move this into core cppcheck instead.. could you look into that? that would be used by a lot more cppcheck users. :-)

mptre · 2025-09-28T18:50:31Z

On Sun, Sep 28, 2025 at 07:59:23AM -0700, Daniel Marjamäki wrote: danmar left a comment (danmar/cppcheck#7843) > Say you have union { uint8_t u8; uint64_t u64 } u = {0} and only operate on u8 would be a false positive. yes true but then it would be pretty redundant to use a union I guess.. I have a good feeling about this checker and would like to move this into core cppcheck instead.. could you look into that? that would be used by a lot more cppcheck users. :-)

Sure, I will take a stab at this.

danmar · 2025-10-01T11:24:17Z

lib/checkunionzeroinit.cpp

+        const Union &u = it->second;
+        if (!u.isLargestMemberFirst()) {
+            const UnionMember *largestMember = u.largestMember();
+            assert(largestMember != nullptr);


The assert macro shall not be used with a constant-expression

This is a false positive. We will fix it in Cppcheck Premium. The expression is not a constant-expression.

lib/checkunionzeroinit.cpp

mptre · 2025-09-29T18:33:32Z

@danmar migrated to cppcheck core. Let me know what you think.

danmar · 2025-10-01T11:10:12Z

lib/tokenize.cpp

            !Token::simpleMatch(tok->tokAt(2), "default :")) {
            Token *tok1 = typeTok->next();
-            if (Token::Match(tok1, "%name% : %num% [;=]"))
+            if (Token::Match(tok1, "%name% : %num% [;=,]"))


good.. but I am not sure about doing this in the same PR.

separate ticket+pr+testcase (in testtokenizer.cpp) would be ideal..

test/testunionzeroinit.cpp

danmar · 2025-10-01T11:17:26Z

lib/checkers.cpp

        {"CheckType::checkLongCast","style"},
        {"CheckType::checkSignConversion","warning"},
        {"CheckType::checkTooBigBitwiseShift","platform"},
+        {"CheckUninitVar::check",""},


this does not seem correct but anyway it is auto generated so will be fixed later..

danmar · 2025-10-01T11:18:29Z

lib/checkunionzeroinit.cpp

@@ -0,0 +1,209 @@
+/*


My feeling is that we could add this in some existing checkers class.. checkother maybe. it is just 1 checker in this class and I doubt we will add more related checkers here later.

I don't find that argument compelling and rather keep the logic separated.

we have more than 100 checkers and we have always grouped checkers into classes we did not write each checker in its own class.

I would find it more difficult to navigate our sourcecode, buildfiles, etc if we had 100's more of checkxyz.cpp , checkxyz.h and testxyz.cpp files..

danmar · 2025-10-01T11:20:49Z

lib/checkunionzeroinit.cpp

+    return Token::simpleMatch(tok, "= { 0 } ;") ||
+           Token::simpleMatch(tok, "= { } ;");


Suggested change

return Token::simpleMatch(tok, "= { 0 } ;") ||

Token::simpleMatch(tok, "= { } ;");

return Token::Match(tok, "= { 0| };");

Does not work as intended...

oops I forgot a space

Suggested change

return Token::simpleMatch(tok, "= { 0 } ;") ||

Token::simpleMatch(tok, "= { } ;");

return Token::Match(tok, "= { 0| } ;");

danmar · 2025-10-01T11:24:17Z

lib/checkunionzeroinit.cpp

+        const Union &u = it->second;
+        if (!u.isLargestMemberFirst()) {
+            const UnionMember *largestMember = u.largestMember();
+            assert(largestMember != nullptr);


The assert macro shall not be used with a constant-expression

This is a false positive. We will fix it in Cppcheck Premium. The expression is not a constant-expression.

sonarqubecloud · 2025-10-01T17:55:22Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

This reverts commit 4823852.

danmar · 2025-10-03T06:54:11Z

oh shit I merged by mistake :-( I revert it with #7868

Reverts #7843

mptre force-pushed the unionzeroinit branch from d730e9b to 99c5739 Compare September 22, 2025 18:05

mptre force-pushed the unionzeroinit branch from 99c5739 to 70bf6db Compare September 23, 2025 17:53

mptre changed the title ~~addons: add unionzeroinit~~ addons: add unionzeroinit (#14150) Sep 23, 2025

mptre commented Sep 23, 2025

View reviewed changes

danmar changed the title ~~addons: add unionzeroinit (#14150)~~ Fix #14150 (addons: add unionzeroinit) Sep 28, 2025

Bitfields may be followed by comma

7344d5f

mptre changed the title ~~Fix #14150 (addons: add unionzeroinit)~~ Fix #14150 (Add unionzeroinit check) Sep 29, 2025

github-advanced-security bot found potential problems Sep 29, 2025

View reviewed changes

mptre force-pushed the unionzeroinit branch 2 times, most recently from 4ff2ed0 to 51d7983 Compare September 29, 2025 18:33

mptre force-pushed the unionzeroinit branch from 51d7983 to afc3bcd Compare September 29, 2025 18:54

danmar reviewed Oct 1, 2025

View reviewed changes

mptre mentioned this pull request Oct 1, 2025

Fix #14170 (Bitfields may be followed by comma) #7865

Merged

Migrate UnionZeroInit to cppcheck core

937add6

mptre force-pushed the unionzeroinit branch from afc3bcd to 937add6 Compare October 1, 2025 17:44

danmar merged commit 4823852 into danmar:main Oct 3, 2025
53 checks passed

danmar added a commit that referenced this pull request Oct 3, 2025

Revert "Fix #14150 (Add unionzeroinit check) (#7843)"

6dc54be

This reverts commit 4823852.

danmar mentioned this pull request Oct 3, 2025

Revert "Fix #14150 (Add unionzeroinit check)" #7868

Merged

danmar added a commit that referenced this pull request Oct 3, 2025

Revert "Fix #14150 (Add unionzeroinit check)" (#7868)

2bebf5d

Reverts #7843

mptre mentioned this pull request Oct 4, 2025

Fix #14150 (Add unionzeroinit check) #7870

Merged


		nested_variables = variable.nameToken.valueType.typeScope.varlist

		# Circumvent what seems to be a bug in which only the last bitfield has

		return Token::simpleMatch(tok, "= { 0 } ;") \|\|
		Token::simpleMatch(tok, "= { } ;");

	return Token::simpleMatch(tok, "= { 0 } ;") \|\|
	Token::simpleMatch(tok, "= { } ;");
	return Token::Match(tok, "= { 0\| };");

	return Token::simpleMatch(tok, "= { 0 } ;") \|\|
	Token::simpleMatch(tok, "= { } ;");
	return Token::Match(tok, "= { 0\| } ;");

Fix #14150 (Add unionzeroinit check) #7843

Fix #14150 (Add unionzeroinit check) #7843

Uh oh!

Conversation

mptre commented Sep 22, 2025

Uh oh!

danmar commented Sep 23, 2025

Uh oh!

mptre commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

danmar commented Sep 28, 2025

Uh oh!

mptre commented Sep 28, 2025 via email

Uh oh!

Check warning

danmar Oct 1, 2025 • 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!

Uh oh!

mptre commented Sep 29, 2025

Uh oh!

danmar Oct 1, 2025 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmar Oct 1, 2025 • 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!

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!

danmar Oct 1, 2025 • 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!

sonarqubecloud bot commented Oct 1, 2025

Quality Gate passed

Uh oh!

Uh oh!

danmar commented Oct 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mptre commented Sep 23, 2025 •

edited

Loading

danmar Oct 1, 2025 •

edited

Loading

danmar Oct 1, 2025 •

edited

Loading

danmar Oct 1, 2025 •

edited

Loading

danmar Oct 1, 2025 •

edited

Loading