A couple of fixes for issues discovered by GCC analyzer #529
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As that's a way to a certain segmentation fault.
Discovered by GCC analyzer:
[74/84] Linking target src/polkitagent/polkit-agent-helper-1
../src/polkitagent/polkitagenthelper-pam.c: In function ‘send_to_helper’:
../src/polkitagent/polkitagenthelper-pam.c:46:10: warning: use of NULL where non-null expected [CWE-476] [-Wanalyzer-null-argument]
46 | len2 = strlen(tmp2);
| ^
[83/84] Linking target src/programs/pkexec
../src/programs/pkexec.c: In function ‘main’:
../src/programs/pkexec.c:817:7: warning: use of NULL ‘command_line’ where non-null expected [CWE-476] [-Wanalyzer-null-argument]
817 | if (strlen(command_line) > 80)
| ^
push_action_and_details() never used the "error" argument internally, so it always remained set to NULL, which would lead to nasty surprises in the error paths. Found by GCC analyzer: [84/84] Linking target src/polkitbackend/polkitd ../src/polkitbackend/polkitbackendduktapeauthority.c: In function ‘polkit_backend_common_js_authority_check_authorization_sync’: ../src/polkitbackend/polkitbackendduktapeauthority.c:1015:7: warning: dereference of NULL ‘error’ [CWE-476] [-Wanalyzer-null-dereference] 1015 | polkit_backend_authority_log (POLKIT_BACKEND_AUTHORITY (authority),
The error object in this case should always be allocated if
polkit_authority_check_authorization_finish() returns NULL, so let's
convey this message to static analyzers as well.
Another potential "solution" for this could be checking the error object
instead of the result one when returning from
polkit_authority_check_authorization_finish() (like it's done in the
respective example - src/examples/cancel.c), but we already have a
precedent of using g_assert() on the error object for this in our
codebase.
This should address following warning from GCC analyzer:
[75/89] Linking target src/polkit/libpolkit-gobject-1.so.0.0.0
../src/polkit/polkitpermission.c: In function ‘changed_check_cb’:
../src/polkit/polkitpermission.c:485:7: warning: dereference of NULL ‘error’ [CWE-476] [-Wanalyzer-null-dereference]
485 | g_warning ("Error checking authorization for action id %s: %s",
| ^
|
Thanks! This should be enabled in Packit configurations too. Please add this line to Packit configurations: |
|
OpenScanHub task shows 4 fixed warnings after LTO was enabled: https://openscanhub.fedoraproject.org/task/25069/log/fixed.html So, it seems to work as expected. Thanks! |
siteshwar
reviewed
Nov 21, 2024
GCC analyzer in some cases depends on information provided by LTO to generate accurate results, but the LTO support in GCC analyzer is still experimental, so it's not enabled by default. In Polkit, however, this seems to work pretty well, so let's tell OSH to build our code with LTO when running GCC analyzer.
mrc0mmand
force-pushed
the
appease-gccs-analyzer
branch
from
b2627ee
to
ab04148
Compare
siteshwar
approved these changes
Nov 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses three issues (and one potential one) found by GCC analyzer after my adventures with OpenScanHub. See the respective commits for more details.