#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 5 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.


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.

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.

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

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?

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

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.

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.

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.

Fields changed

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

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

Fields changed

rhbz: => 0

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

2 years ago

Login to comment on this ticket.

Metadata