Issue #2258: [src/providers/krb5/krb5_common.c:418] -> [src/providers/krb5/krb5_common.c:418]: (style) Same expression on both sides of '||'. - sssd

SSSD / sssd

#2258 [src/providers/krb5/krb5_common.c:418] -> [src/providers/krb5/krb5_common.c:418]: (style) Same expression on both sides of '||'.

Closed: Fixed None Opened 10 years ago by dcb.

Source code is

if (realm == NULL || *realm == '\0' || server == NULL || *server == '\0' ||
    service == NULL || service == '\0') {
    DEBUG(1, ("Missing or empty realm, server or service.\n"));
    return EINVAL;
}

Maybe better code would be

if (realm == NULL || *realm == '\0' || server == NULL || *server == '\0' ||
    service == NULL || *service == '\0') {
    DEBUG(1, ("Missing or empty realm, server or service.\n"));
    return EINVAL;
}

I found this bug using cppcheck, a static analysis tool.

jhrozek commented 10 years ago

Thanks, nice catch! Would you like to send a git-formatted patch to sssd-devel? Fame and glory awaits you!

The list is normally moderated, so you can either subscribe or I can let the mail through manually, no big deal.

dcb commented 10 years ago

Thanks, nice catch!

Cheers.

Would you like to send a git-formatted patch to sssd-devel? Fame and glory awaits you!

Thanks but no. I've got enough Fame and Glory.

You might like to look closer at these three as well.

[src/responder/nss/nsssrv_services.c:1075]: (style) Array index 'i' is used befo
re limits check.
[src/responder/nss/nsssrv_services.c:912]: (style) Array index 'i' is used befor
e limits check.
[src/responder/nss/nsssrv_services.c:947]: (style) Array index 'i' is used befor
e limits check.

Past those three, feel free to download cppcheck from sourceforge and
try it out for yourself on sssd code.

jhrozek commented 10 years ago

Replying to [comment:2 dcb]:

[src/responder/nss/nsssrv_services.c:1075]: (style) Array index 'i' is used befo
re limits check.
[src/responder/nss/nsssrv_services.c:912]: (style) Array index 'i' is used befor
e limits check.
[src/responder/nss/nsssrv_services.c:947]: (style) Array index 'i' is used befor
e limits check.

Past those three, feel free to download cppcheck from sourceforge and
try it out for yourself on sssd code.

Thanks again, Lukas (CC-ed) is out team's best expert in static analysis tool, so I'll leave experimenting with cppcheck to him :-)

cc: => lslebodn

lslebodn commented 10 years ago

Replying to [comment:2 dcb]:

Thanks, nice catch!

Cheers.

Would you like to send a git-formatted patch to sssd-devel? Fame and glory awaits you!

Thanks but no. I've got enough Fame and Glory.

You might like to look closer at these three as well.

[src/responder/nss/nsssrv_services.c:1075]: (style) Array index 'i' is used befo
re limits check.
[src/responder/nss/nsssrv_services.c:912]: (style) Array index 'i' is used befor
e limits check.
[src/responder/nss/nsssrv_services.c:947]: (style) Array index 'i' is used befor
e limits check.

Past those three, feel free to download cppcheck from sourceforge and
try it out for yourself on sssd code.

Which version of sssd did you test? Did you test sssd from git? (which branch and which commit).

I am aware of few warnings reported by cppcheck

Error: CPPCHECK_WARNING: [#def1]
sssd-1.11.90/src/responder/nss/nsssrv_services.c:773: error[uninitvar]: Uninitialized variable: body

Error: CPPCHECK_WARNING: [#def2]
sssd-1.11.90/src/responder/nss/nsssrv_services.c:776: error[uninitvar]: Uninitialized variable: body

Error: CPPCHECK_WARNING: [#def3]
sssd-1.11.90/src/sss_client/pam_test_client.c:94: error[memleak]: Memory leak: action

I tested sssd master few weeks ago. The 1st and the 2nd warnings are false positive. And the last one is irrelevant, because pam_test_client is only test command line utility.

Which version of cppcheck did you use?

lslebodn commented 10 years ago

I am not able to find difference in the ticket description.

dcb commented 10 years ago

I tested sssd master few weeks ago. The 1st and the 2nd warnings are false positive.
And the last one is irrelevant, because pam_test_client is only test command line utility.

Which version of cppcheck did you use?

Dev trunk, but I don't think that's important.
Latest version, 1.63, should be good also.

I don't know which flags were used on cppcheck, but I strongly recommend "--enable=all",
to encourage it to say everything it can find.

Not everything it can find is worth fixing.

lslebodn commented 10 years ago

Replying to [comment:6 dcb]:

I tested sssd master few weeks ago. The 1st and the 2nd warnings are false positive.
And the last one is irrelevant, because pam_test_client is only test command line utility.

Which version of cppcheck did you use?

Dev trunk, but I don't think that's important.
Latest version, 1.63, should be good also.

I don't know which flags were used on cppcheck, but I strongly recommend "--enable=all",
to encourage it to say everything it can find.
I will try cppcheck with "--enable=all", but in my experience enabling all warnings in static analysers produce more false positives.

Not everything it can find is worth fixing.

You didn't reply to all my questions:

Which version of sssd did you test?
Did you test sssd from git? (which branch and which commit)
Could you use better formatting in the ticket description?

I am pretty sure you did not test master branch.

dcb commented 10 years ago

Replying to [comment:7 lslebodn]:

Which version of sssd did you test?

sssd-1.11.4 aka latest version in fedora rawhide.

Did you test sssd from git? (which branch and which commit)

Nope.

Could you use better formatting in the ticket description?

Sure.

I am pretty sure you did not test master branch.

Agreed. I tested only the latest version in fedora.

Now that --enable=all is known, it should be trivial
to run cppcheck over latest dev code.

lslebodn commented 10 years ago

Fields changed

owner: somebody => lslebodn
patch: 0 => 1
status: new => assigned

jhrozek commented 10 years ago

master:
- a842b7e
- 2aa96da
- a602b0d
- cfaa16f

milestone: NEEDS_TRIAGE => SSSD 1.12 beta
resolution: => fixed
status: assigned => closed

jhrozek commented 10 years ago

Fields changed

rhbz: => 0

Metadata Update from @dcb:
- Issue assigned to lslebodn
- Issue set to the milestone: SSSD 1.12 beta

7 years ago

pbrezina commented 3 years ago

SSSD is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in SSSD's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/SSSD/sssd/issues/3300

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata

Assignee

lslebodn

Tags

None

Blocking

None

Depending on

None

Priority

minor

Milestone

SSSD 1.12 beta

type

defect

component

SSSD

version

1.11.4

selected

None

testsupdated

patch

rhbz

design_review

review

changelog

None

keywords

None

coverity

None

mark

None

blocking

None

design

None

sensitive

None

lslebodn

blockedby

None

feature_milestone

None

SSSD / sssd

Source Code

Documentation

#2258 [src/providers/krb5/krb5_common.c:418] -> [src/providers/krb5/krb5_common.c:418]: (style) Same expression on both sides of '||'. Closed: Fixed None Opened 10 years ago by dcb.

Metadata

#2258 [src/providers/krb5/krb5_common.c:418] -> [src/providers/krb5/krb5_common.c:418]: (style) Same expression on both sides of '||'.

Closed: Fixed None Opened 10 years ago by dcb.