[Swan-dev] oddity in ikev2_process_child_sa_pl

[Andrew Cagney]

Occam's razor.  The code is, after all working.

    accepted_dh == NULL ? "no DH" : accepted_dh->common.fqn);

is to ensure that this code snippet (which means in isolation and even
when moved elsewhere) doesn't dump core.

There's a way to tell coverity to ignore this (or just do it as a
human), I'd be using that feature.


On 3 June 2018 at 18:52, Neal P. Murphy <neal.p.murphy at alum.wpi.edu> wrote:
> 'Inverting' the logic yields
>
>                 if ( ! (accepted_dh == NULL || accepted_dh == st->st_pfs_group)) {
>
> which doesn't look right either. Mayhap it should be
>
> /*
>  * Verify the pointer isn't null before comparing.
>  * The error is that either there's no DH or it doesn't match what was negotiated.
>  * Otherwise the DH is stored.
>  */
>                 if ( accepted_dh == NULL || accepted_dh != st->st_pfs_group) {
>
> But this still doesn't look quite right. If st->st_pfs_group is null, you want to save the DH, right?
>
> Maybe it wants to be
>
>                 if ( accepted_dh == NULL || (st->st_pfs_group != NULL && accepted_dh != st->st_pfs_group)) {
>
>
> N
>
> On Sun, 3 Jun 2018 18:18:28 -0400 (EDT)
> "D. Hugh Redelmeier" <hugh at mimosa.com> wrote:
>
>>         /*
>>          * Update/check the PFS.
>>        *
>>          * For the responder, go with what ever was negotiated.        For
>>        * the initiator, check what was negotiated against what was
>>        * sent.
>>        */
>>       const struct oakley_group_desc *accepted_dh = proto_info->attrs.transattrs.ta_dh;
>>       switch (st->st_sa_role) {
>>       case SA_INITIATOR:
>>               pexpect(expect_accepted);
>>               if (accepted_dh != NULL && accepted_dh != st->st_pfs_group) {
>>                       loglog(RC_LOG_SERIOUS,
>>                              "expecting %s but remote's accepted proposal includes %s",
>>                              st->st_pfs_group == NULL ? "no DH" : st->st_pfs_group->common.fqn,
>>                              accepted_dh == NULL ? "no DH" : accepted_dh->common.fqn);
>>                         return STF_FAIL + v2N_NO_PROPOSAL_CHOSEN;
>>               }
>>                 st->st_pfs_group = accepted_dh;
>>               break;
>>
>> coverity noticed that
>>                              accepted_dh == NULL ? "no DH" : accepted_dh->common.fqn);
>> was silly because we know that accepted_dh is not NULL (the preceding if checks).
>>
>> I'm wondering, whether the test is correct.  Should it be || instead of &&?  It's not clear to me.
>> _______________________________________________
>> Swan-dev mailing list
>> Swan-dev at lists.libreswan.org
>> https://lists.libreswan.org/mailman/listinfo/swan-dev
>
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev