Sign compare checking

[William Brown]

Hi,

I'm going through the list of sign-compare errors from gcc, and putting
in the appropriate casts (uint -> int normally).

I found in filter.c:

        /* openldap does not return LBER_END_OF_SEQORSET -
           so check for len == -1 - openldap ber_next_element will not
set
           len if it has reached the end, and -1 is not a valid value
           for a real len */
        if ( (tag != LBER_END_OF_SEQORSET) && (len != -1) && (*fstr !=
NULL) ) {
                LDAPDebug( LDAP_DEBUG_ANY, "   error parsing filter list
\n", 0, 0, 0 );
                slapi_ch_free((void**)fstr );
        }


However, len is a ber_len_t, which is a typedef uint. It's actually
impossible for this value to hold a -1.

So either, this is a bug in the way openldap uses the ber_len_t type, we
have a mistake in our logic, or something else hokey is going on. 

I would like to update this to:

if ( (tag != LBER_END_OF_SEQORSET) && (len == 0) && (*fstr != NULL) )

Or even:

if ( (tag != LBER_END_OF_SEQORSET) && (*fstr != NULL) )

What do you think of this assessment given the ber_len_t type? 

-- 
Sincerely,

William Brown
Software Engineer
Red Hat, Brisbane

[William Brown]

>  
>  So either, this is a bug in the way openldap uses the ber_len_t type, we
>  have a mistake in our logic, or something else hokey is going on. 
>  
>  I would like to update this to:
>  
>  if ( (tag != LBER_END_OF_SEQORSET) && (len == 0) && (*fstr != NULL) )
>  
>  Or even:
>  
>  if ( (tag != LBER_END_OF_SEQORSET) && (*fstr != NULL) )
>  
>  What do you think of this assessment given the ber_len_t type?  
Looks like it's intentional by the openldap team. There are some other
areas for this problem. Specifically:

int ber_printf(BerElement *ber, const char *fmt, ...);

lber.h:79:#define LBER_ERROR            ((ber_tag_t) -1)

We check if (ber_printf(...) != LBER_ERROR)

Of course, we can't satisfy either. We can't cast the LBER_ERROR from
uint -> int without changing the value of it, and we can't cast the
output of ber_printf from int -> uint, again, without potentially
changing the value of it. So it seems that the openldap library may be
impossible to satisfy the gcc type checking with -Wsign-compare.

For now, I may just avoid these in my fixes, as it seems like a whole
set of landmines I want to avoid ... 

-- 
Sincerely,

William Brown
Software Engineer
Red Hat, Brisbane

[Rich Megginson]

On 08/28/2016 11:13 PM, William Brown wrote:
> > So either, this is a bug in the way openldap uses the ber_len_t
> type, we
> > have a mistake in our logic, or something else hokey is going on.
> >
> > I would like to update this to:
> >
> > if ( (tag != LBER_END_OF_SEQORSET) && (len == 0) && (*fstr != NULL)
> )
> >
> > Or even:
> >
> > if ( (tag != LBER_END_OF_SEQORSET) && (*fstr != NULL) )
> >
> > What do you think of this assessment given the ber_len_t type?
>  Looks like it's intentional by the openldap team. There are some other
>  areas for this problem. Specifically:
>
>  int ber_printf(BerElement *ber, const char *fmt, ...);
>
>  lber.h:79:#define LBER_ERROR            ((ber_tag_t) -1)
>
>  We check if (ber_printf(...) != LBER_ERROR)
>
>  Of course, we can't satisfy either. We can't cast the LBER_ERROR from
>  uint -> int without changing the value of it, and we can't cast the
>  output of ber_printf from int -> uint, again, without potentially
>  changing the value of it. So it seems that the openldap library may be
>  impossible to satisfy the gcc type checking with -Wsign-compare.
>
>  For now, I may just avoid these in my fixes, as it seems like a whole
>  set of landmines I want to avoid ... 
Part of the problem is that we wanted to support being able to use both 
mozldap and openldap, without too much "helper" code/macros/#ifdef 
MOZLDAP/etc.  It looks as though this is a place where we need to have 
some sort of helper.

(as for why we still support mozldap - we still need an ldap c sdk that 
supports NSS for crypto until we can fix that in the server. Once we 
change 389 so that it can use openldap with openssl/gnutls for crypto, 
we should consider deprecating support for mozldap.)

>  --
>  389-devel mailing list
>  389-devel(a)lists.fedoraproject.org
>  https://lists.fedoraproject.org/admin/lists/[e-mail]...

[William Brown]

>  >
>  > int ber_printf(BerElement *ber, const char *fmt, ...);
>  >
>  > lber.h:79:#define LBER_ERROR            ((ber_tag_t) -1)
>  >
>  > We check if (ber_printf(...) != LBER_ERROR)
>  >
>  > Of course, we can't satisfy either. We can't cast the LBER_ERROR from
>  > uint -> int without changing the value of it, and we can't cast the
>  > output of ber_printf from int -> uint, again, without potentially
>  > changing the value of it. So it seems that the openldap library may be
>  > impossible to satisfy the gcc type checking with -Wsign-compare.
>  >
>  > For now, I may just avoid these in my fixes, as it seems like a whole
>  > set of landmines I want to avoid ...
>   
>  
>  (as for why we still support mozldap - we still need an ldap c sdk that 
>  supports NSS for crypto until we can fix that in the server. Once we 
>  change 389 so that it can use openldap with openssl/gnutls for crypto, 
>  we should consider deprecating support for mozldap.) 
Actually, all of this is specific to openldap only. Nothing about
mozldap at all. 

So I think I'll just have to ignore the GCC warnings about this when
it's related to openldap ber library. 

-- 
Sincerely,

William Brown
Software Engineer
Red Hat, Brisbane