On Mon, 2023-08-28 at 17:13 +0200, Benjamin Priour wrote:
> Hi,
>
> Test gcc.dg/analyzer/pr94362-1.c actually has an additional
> null_deref
> > warning in C++, which is not affected by exceptions
> > or optimizations. I will keep updated on that one. Note that no
> > warnings
> > are emitted for this test in C.
> >
> > analyzer/pr94362-1.c: In function ‘const EVP_PKEY_ASN1_METHOD*
> > EVP_PKEY_asn1_find_str(ENGINE**, const char*, int)’:
> > analyzer/pr94362-1.c:55:16: warning: dereference of NULL ‘ameth’
> > [CWE-476]
> > [-Wanalyzer-null-dereference]
> > analyzer/pr94362-1.c:39:29: note: (1) entry to
> > ‘EVP_PKEY_asn1_find_str’
> > analyzer/pr94362-1.c:42:31: note: (2) ‘ameth’ is NULL
> > analyzer/pr94362-1.c:53:43: note: (3) following ‘true’ branch...
> > analyzer/pr94362-1.c:54:31: note: (4) ...to here
> > analyzer/pr94362-1.c:54:31: note: (5) calling ‘EVP_PKEY_asn1_get0’
> > from
> > ‘EVP_PKEY_asn1_find_str’
> > analyzer/pr94362-1.c:29:29: note: (6) entry to ‘EVP_PKEY_asn1_get0’
> > analyzer/pr94362-1.c:32:53: note: (7) ‘0’ is NULL
> > analyzer/pr94362-1.c:54:31: note: (8) returning to
> > ‘EVP_PKEY_asn1_find_str’ from ‘EVP_PKEY_asn1_get0’
> > analyzer/pr94362-1.c:54:31: note: (9) ‘ameth’ is NULL
> > analyzer/pr94362-1.c:55:16: note: (10) dereference of NULL ‘ameth’
> >
> >
> > Cheers,
> > Benjamin.
> >
> >
> As promised a quick update on that front. I've managed to pinpoint
> the
> difference between the C and C++.
> Compiling with -fno-exceptions -O0 the two IPA's seen by the analyzer
> are
> nearly identical, if not for one difference.
>
> const EVP_PKEY_ASN1_METHOD *EVP_PKEY_asn1_find_str(ENGINE **pe, const
> char
> *str,
> int len)
> {
> int i;
> const EVP_PKEY_ASN1_METHOD *ameth = (const EVP_PKEY_ASN1_METHOD *)
> ((void
> *)0);
> ...
> for (i = EVP_PKEY_asn1_get_count(); i-- > 0;) {
> ameth = EVP_PKEY_asn1_get0(i); /* At this point, i >= 0 */
> if (ameth->pkey_flags & 0x1)
> continue;
> return ameth;
> }
> ...
> }
>
> IPA when compiled by as C:
>
> <bb 6> :
> i_23 = EVP_PKEY_asn1_get_count ();
> goto <bb 10>; [INV]
>
> <bb 7> :
> ameth_27 = EVP_PKEY_asn1_get0 (i_24);
> _2 = ameth_27->pkey_flags;
> _3 = _2 & 1;
> if (_3 != 0)
> goto <bb 8>; [INV]
> else
> goto <bb 9>; [INV]
>
> <bb 8> :
> // predicted unlikely by continue predictor.
> goto <bb 10>; [INV]
>
> <bb 9> :
> _28 = ameth_27;
> goto <bb 12>; [INV]
>
> <bb 10> :
> # i_5 = PHI <i_23(6), i_24(8)>
> i.4_4 = i_5;
> i_24 = i.4_4 + -1;
> if (i.4_4 > 0)
> goto <bb 7>; [INV]
> else
> goto <bb 11>; [INV]
>
> ... and as C++
>
> <bb 6> :
> i_23 = EVP_PKEY_asn1_get_count ();
> goto <bb 10>; [INV]
>
> <bb 7> :
> ameth_28 = EVP_PKEY_asn1_get0 (i_24);
> _2 = ameth_28->pkey_flags;
> _3 = _2 & 1;
> if (_3 != 0)
> goto <bb 8>; [INV]
> else
> goto <bb 9>; [INV]
>
> <bb 8> :
> // predicted unlikely by continue predictor.
> goto <bb 10>; [INV]
>
> <bb 9> :
> _29 = ameth_28;
> goto <bb 12>; [INV]
>
> <bb 10> :
> # i_5 = PHI <i_23(6), i_24(8)>
> i.5_4 = i_5;
> i_24 = i.5_4 + -1;
> retval.4_25 = i.5_4 > 0; /* Difference here. C++ stores the
> comparison's result before using it */
> if (retval.4_25 != 0)
> goto <bb 7>; [INV]
> else
> goto <bb 11>; [INV]
>
> This slight difference causes i_24 to not be part of an equivalence
> class
> in C++, although it is part of one in C.
> Therefore when calling ameth_28 = EVP_PKEY_asn1_get0 (i_24);, i_24
> won't
> appear as constrained in C++,
> although we know it to be positive. Info is lost.
> Further down the line, because of this missing ec
> constraint_manager::eval_condition will evaluate to UNKNOWN,
> and the pending diagnostic path won't be refused but accepted as
> feasible
> in C++.
>
> constraints and equivalence classes in C:
>
> ec2: {(CONJURED(_8 = sk_EVP_PKEY_ASN1_METHOD_num (app_methos.1_2);,
> _8)+(int)1)}
> ec3: {(int)0 == [m_constant]'0'} /* int since constraining 'int i_24'
> */
> ec4: {CONJURED(_8 = sk_EVP_PKEY_ASN1_METHOD_num (app_methos.1_2);,
> _8)} /*
> This is the ec of i_24 */
> ec5: {(int)-1 == [m_constant]'-1'}
> constraints:
> 1: ec5 < ec4
> 2: ec3 < ec2
>
> ... vs C++'s:
>
> ec2: {((CONJURED(_8 = sk_EVP_PKEY_ASN1_METHOD_num (app_methos.1_2);,
> _8)+(int)1)>(int)0)}
> ec3: {(bool)0 == [m_constant]'0'} /* bool instead of int cuz
> constraining
> 'bool retval.4_25' */
> constraints:
> 1: ec2 != ec3
>
> As you can see, i_24 equivalence class does not appear in C++.
> I'm considering if in a condition lhs or rhs is a logical binop
> svalue, we
> should unwrap one level, so that the condition's constraint is
> actually
> applied on
> the inner svalue.
Aha - good debugging!
Your proposed fix sounds worth investigating. Let me know if you'd
like help with it.
Thanks
Dave
