Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GCC v14 [-Wanalyzer-null-dereference] warnings in Kerberos #1983

Closed
wants to merge 24 commits into from

Conversation

huaraz
Copy link
Contributor

@huaraz huaraz commented Jan 17, 2025

src/acl/external/kerberos_ldap_group/support_sasl.cc:190:17: error:
dereference of NULL 'defs' [CWE-476] [-Wanalyzer-null-dereference]

src/auth/negotiate/kerberos/negotiate_kerberos_pac.cc:235:19: error:
dereference of NULL 'Rids' [CWE-476] [-Wanalyzer-null-dereference]

@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Jan 17, 2025
@rousskov rousskov changed the title Fix static code analysis findings Fix GCC v14 [-Wanalyzer-null-dereference] warnings in Kerberos Jan 17, 2025
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for improving this code.

I made PR title more specific and fixed PR description formatting. I also committed (without testing) two trivial adjustments to avoid adding new NULLs to C++ code. Please check!

AFAICT, the first warning is false -- lutil_sasl_defaults() never returns nil because our xmalloc() never returns nil. However, resource freeing functions like lutil_sasl_freedefs() should be written like delete (i.e. do nothing when the pointer is nil). Thus, this PR improves that code.

I did not have enough time to check whether the other warning is false.

The log containing these and other GCC v14 static analyzer warnings is available in squid-dev archives.

@squid-anubis squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Jan 17, 2025
@rousskov rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Jan 17, 2025
@huaraz
Copy link
Contributor Author

huaraz commented Jan 18, 2025

Thank you for the comments. I also did not a full check if it is a false positive, but to avoid analyser warnings I added a simple check.

@yadij yadij added backport-to-v6 maintainer has approved these changes for v6 backporting and removed S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Jan 19, 2025
squid-anubis pushed a commit that referenced this pull request Jan 19, 2025
    src/acl/external/kerberos_ldap_group/support_sasl.cc:190:17: error:
    dereference of NULL 'defs' [CWE-476] [-Wanalyzer-null-dereference]

    src/auth/negotiate/kerberos/negotiate_kerberos_pac.cc:235:19: error:
    dereference of NULL 'Rids' [CWE-476] [-Wanalyzer-null-dereference]
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Jan 19, 2025
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Jan 19, 2025
squidadm pushed a commit to squidadm/squid that referenced this pull request Jan 19, 2025
…-cache#1983)

    src/acl/external/kerberos_ldap_group/support_sasl.cc:190:17: error:
    dereference of NULL 'defs' [CWE-476] [-Wanalyzer-null-dereference]

    src/auth/negotiate/kerberos/negotiate_kerberos_pac.cc:235:19: error:
    dereference of NULL 'Rids' [CWE-476] [-Wanalyzer-null-dereference]
@squidadm squidadm removed the backport-to-v6 maintainer has approved these changes for v6 backporting label Jan 19, 2025
@squidadm
Copy link
Collaborator

queued for backport to v6

kinkie pushed a commit that referenced this pull request Jan 19, 2025
    src/acl/external/kerberos_ldap_group/support_sasl.cc:190:17: error:
    dereference of NULL 'defs' [CWE-476] [-Wanalyzer-null-dereference]

    src/auth/negotiate/kerberos/negotiate_kerberos_pac.cc:235:19: error:
    dereference of NULL 'Rids' [CWE-476] [-Wanalyzer-null-dereference]
@kinkie kinkie mentioned this pull request Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants