[PATCH SYSDB: Fix handling of version in sysdb_cache_connect_helper

[Lukas Slebodnik]

ehlo,

Clang static analyzer assume that ldb_search can found
0 entries in the tree "cn=sysdb". Thenvariable version
could be used uninitialized.

We cannot get to such state in sssd but we already handle
a case for more then one entry.

LS

[Fabiano Fidêncio]

Lukaš,

On Tue, Aug 30, 2016 at 4:54 PM, Lukas Slebodnik  wrote:
> ehlo,
>
> Clang static analyzer assume that ldb_search can found
> 0 entries in the tree "cn=sysdb". Thenvariable version
> could be used uninitialized.
>
> We cannot get to such state in sssd but we already handle
> a case for more then one entry.

I don't think this is the right approach as res->count == 0 seems to
be a valid case for a newly created database (please, correct me if
I'm wrong).

Best Regards,
--
Fabiano Fidêncio

[Lukas Slebodnik]

On (30/08/16 16:59), Fabiano Fidêncio wrote:
> Lukaš,
>
> On Tue, Aug 30, 2016 at 4:54 PM, Lukas Slebodnik  wrote:
> > ehlo,
> >
> > Clang static analyzer assume that ldb_search can found
> > 0 entries in the tree "cn=sysdb". Thenvariable version
> > could be used uninitialized.
> >
> > We cannot get to such state in sssd but we already handle
> > a case for more then one entry.
>
> I don't think this is the right approach as res->count == 0 seems to
> be a valid case for a newly created database (please, correct me if
> I'm wrong).

Agree

I should have tried to run unit test before sending a patch

LS

[Lukas Slebodnik]

On (30/08/16 17:07), Lukas Slebodnik wrote:
> On (30/08/16 16:59), Fabiano Fidêncio wrote:
> > Lukaš,
> >
> > On Tue, Aug 30, 2016 at 4:54 PM, Lukas Slebodnik  wrote:
> > > ehlo,
> > >
> > > Clang static analyzer assume that ldb_search can found
> > > 0 entries in the tree "cn=sysdb". Thenvariable version
> > > could be used uninitialized.
> > >
> > > We cannot get to such state in sssd but we already handle
> > > a case for more then one entry.
> >
> > I don't think this is the right approach as res->count == 0 seems to
> > be a valid case for a newly created database (please, correct me if
> > I'm wrong).
>
> Agree
>
> I should have tried to run unit test before sending a patch

I looked deeper to the clang report
And there are wrong assumption that output variable
"version" is not initialized if function sysdb_cache_connect
returns ERR_SYSDB_VERSION_TOO_OLD or ERR_SYSDB_VERSION_TOO_NEW

The reality is that output variable "version" is initialized
especially for these two case.

It is a false positive but we might suppress the warning
with initializing variable to NULL.

Attached is a gzipped html report from clang

LS

[Fabiano Fidêncio]

On Thu, Sep 1, 2016 at 5:12 PM, Lukas Slebodnik  wrote:
> On (30/08/16 17:07), Lukas Slebodnik wrote:
> > On (30/08/16 16:59), Fabiano Fidêncio wrote:
> > > Lukaš,
> > >
> > > On Tue, Aug 30, 2016 at 4:54 PM, Lukas Slebodnik  wrote:
> > > > ehlo,
> > > >
> > > > Clang static analyzer assume that ldb_search can found
> > > > 0 entries in the tree "cn=sysdb". Thenvariable version
> > > > could be used uninitialized.
> > > >
> > > > We cannot get to such state in sssd but we already handle
> > > > a case for more then one entry.
> > >
> > > I don't think this is the right approach as res->count == 0 seems to
> > > be a valid case for a newly created database (please, correct me if
> > > I'm wrong).
> >
> > Agree
> >
> > I should have tried to run unit test before sending a patch
>
> I looked deeper to the clang report
> And there are wrong assumption that output variable
> "version" is not initialized if function sysdb_cache_connect
> returns ERR_SYSDB_VERSION_TOO_OLD or ERR_SYSDB_VERSION_TOO_NEW
>
> The reality is that output variable "version" is initialized
> especially for these two case.
>
> It is a false positive but we might suppress the warning
> with initializing variable to NULL.

Indeed!
Are you planning to submit this one-liner for review as well?

Best Regards,

[Lukas Slebodnik]

On (01/09/16 17:22), Fabiano Fidêncio wrote:
> On Thu, Sep 1, 2016 at 5:12 PM, Lukas Slebodnik  wrote:
> > On (30/08/16 17:07), Lukas Slebodnik wrote:
> > > On (30/08/16 16:59), Fabiano Fidêncio wrote:
> > > > Lukaš,
> > > >
> > > > On Tue, Aug 30, 2016 at 4:54 PM, Lukas Slebodnik  wrote:
> > > > > ehlo,
> > > > >
> > > > > Clang static analyzer assume that ldb_search can found
> > > > > 0 entries in the tree "cn=sysdb". Thenvariable version
> > > > > could be used uninitialized.
> > > > >
> > > > > We cannot get to such state in sssd but we already handle
> > > > > a case for more then one entry.
> > > >
> > > > I don't think this is the right approach as res->count == 0 seems to
> > > > be a valid case for a newly created database (please, correct me if
> > > > I'm wrong).
> > >
> > > Agree
> > >
> > > I should have tried to run unit test before sending a patch
> >
> > I looked deeper to the clang report
> > And there are wrong assumption that output variable
> > "version" is not initialized if function sysdb_cache_connect
> > returns ERR_SYSDB_VERSION_TOO_OLD or ERR_SYSDB_VERSION_TOO_NEW
> >
> > The reality is that output variable "version" is initialized
> > especially for these two case.
> >
> > It is a false positive but we might suppress the warning
> > with initializing variable to NULL.
>
> Indeed!
> Are you planning to submit this one-liner for review as well?

I can.

LS

[Fabiano Fidêncio]

On Thu, Sep 1, 2016 at 6:02 PM, Lukas Slebodnik  wrote:
> On (01/09/16 17:22), Fabiano Fidêncio wrote:
> > On Thu, Sep 1, 2016 at 5:12 PM, Lukas Slebodnik  wrote:
> > > On (30/08/16 17:07), Lukas Slebodnik wrote:
> > > > On (30/08/16 16:59), Fabiano Fidêncio wrote:
> > > > > Lukaš,
> > > > >
> > > > > On Tue, Aug 30, 2016 at 4:54 PM, Lukas Slebodnik  wrote:
> > > > > > ehlo,
> > > > > >
> > > > > > Clang static analyzer assume that ldb_search can found
> > > > > > 0 entries in the tree "cn=sysdb". Thenvariable version
> > > > > > could be used uninitialized.
> > > > > >
> > > > > > We cannot get to such state in sssd but we already handle
> > > > > > a case for more then one entry.
> > > > >
> > > > > I don't think this is the right approach as res->count == 0 seems to
> > > > > be a valid case for a newly created database (please, correct me if
> > > > > I'm wrong).
> > > >
> > > > Agree
> > > >
> > > > I should have tried to run unit test before sending a patch
> > >
> > > I looked deeper to the clang report
> > > And there are wrong assumption that output variable
> > > "version" is not initialized if function sysdb_cache_connect
> > > returns ERR_SYSDB_VERSION_TOO_OLD or ERR_SYSDB_VERSION_TOO_NEW
> > >
> > > The reality is that output variable "version" is initialized
> > > especially for these two case.
> > >
> > > It is a false positive but we might suppress the warning
> > > with initializing variable to NULL.
> >
> > Indeed!
> > Are you planning to submit this one-liner for review as well?
>
> I can.

Please, just fix a typo before pushing:
(...) to NULL suppress" -> "(...) to NULL suppresses".

Acked-by: Fabiano Fid6encio

Best Regards,

[Lukas Slebodnik]

On (01/09/16 18:15), Fabiano Fidêncio wrote:
> On Thu, Sep 1, 2016 at 6:02 PM, Lukas Slebodnik  wrote:
> > On (01/09/16 17:22), Fabiano Fidêncio wrote:
> > > On Thu, Sep 1, 2016 at 5:12 PM, Lukas Slebodnik  wrote:
> > > > On (30/08/16 17:07), Lukas Slebodnik wrote:
> > > > > On (30/08/16 16:59), Fabiano Fidêncio wrote:
> > > > > > Lukaš,
> > > > > >
> > > > > > On Tue, Aug 30, 2016 at 4:54 PM, Lukas Slebodnik  wrote:
> > > > > > > ehlo,
> > > > > > >
> > > > > > > Clang static analyzer assume that ldb_search can found
> > > > > > > 0 entries in the tree "cn=sysdb". Thenvariable version
> > > > > > > could be used uninitialized.
> > > > > > >
> > > > > > > We cannot get to such state in sssd but we already handle
> > > > > > > a case for more then one entry.
> > > > > >
> > > > > > I don't think this is the right approach as res->count == 0 seems to
> > > > > > be a valid case for a newly created database (please, correct me if
> > > > > > I'm wrong).
> > > > >
> > > > > Agree
> > > > >
> > > > > I should have tried to run unit test before sending a patch
> > > >
> > > > I looked deeper to the clang report
> > > > And there are wrong assumption that output variable
> > > > "version" is not initialized if function sysdb_cache_connect
> > > > returns ERR_SYSDB_VERSION_TOO_OLD or ERR_SYSDB_VERSION_TOO_NEW
> > > >
> > > > The reality is that output variable "version" is initialized
> > > > especially for these two case.
> > > >
> > > > It is a false positive but we might suppress the warning
> > > > with initializing variable to NULL.
> > >
> > > Indeed!
> > > Are you planning to submit this one-liner for review as well?
> >
> > I can.
>
> Please, just fix a typo before pushing:
> (...) to NULL suppress" -> "(...) to NULL suppresses".

Nice catch
Changed.
> Acked-by: Fabiano Fid6encio

http://sssd-ci.duckdns.org/logs/job/52/94/summary.html

master:
* 3f6aecfe5061e165c10829142854ec7189029407

LS