[Swan-dev] why there were useless assignments

[Andrew Cagney]

Perhaps we need a function or macro called coverity_eat_this() that
eats the values passed to it - I'm likely to continue writing code
like what Hugh cited (and if I need to touch that code I'll likely
restore the assignments).  So what great security risk do unused
values pose.


On Wed, 20 Feb 2019 at 15:59, Paul Wouters <paul at nohats.ca> wrote:
>
> On Wed, 20 Feb 2019, D. Hugh Redelmeier wrote:
>
> > Andrew has introduced several.  Here's one:
> >
> > /home/hugh/libreswan/lib/libswan/proposals.c:401:21: warning: Value stored to 'ps' is never read
> >               lswlogs(log, as); ps = "-"; as = "+";
> >                                  ^    ~~~
> >
> > I like this useless assignment.  As I argued below.
> >
> > There are more.
> >
> > Is it still our policy to delete them?  I'd like to revert the policy
> > and revert the commits below.
>
> I'd like to not get new (old!) coverity reports I need to justify....
>
> Paul
>
> > | From: D. Hugh Redelmeier <hugh at mimosa.com>
> > | To: Libreswan Development List <swan-dev at lists.libreswan.org>
> > | Date: Wed, 5 Dec 2018 18:06:17 -0500 (EST)
> > |
> > | | commit c1f6a2bf6041abb5df918fcfd3fa118bb7c761c7
> > | | Author: Paul Wouters <pwouters at redhat.com>
> > | | Date:  Tue Dec 4 12:34:57 2018 -0500
> > | |
> > | |     libswan: lswlog_proposal_info() do not set variable sep when no longer used afterwards
> > |
> > | | commit f0b2361d0e6b20e22c7b5fe77dfe8cf1555901bc
> > | | Author: Paul Wouters <pwouters at redhat.com>
> > | | Date:   Tue Dec 4 12:33:29 2018 -0500
> > | |
> > | |     pluto: ikev2_decode_peer_id_and_certs_counted() don't set c when no longer used
> > |
> > | | commit f48425918e69c15edd8d526cbe4b396373d010d3
> > | | Author: Paul Wouters <pwouters at redhat.com>
> > | | Date:   Tue Dec 4 12:30:51 2018 -0500
> > | |
> > | |     pluto: calc_skeyseed_v2() don't set next_byte when it won't be used anymore
> > |
> > | | commit ce10f9bbaa4f3e0c1926e1a87c75d336b922aa38
> > | | Author: Paul Wouters <pwouters at redhat.com>
> > | | Date:   Tue Dec 4 11:30:01 2018 -0500
> > | |
> > | |     pluto: initiate_ondemand_body() fix unused setting of loggedit
> > |
> > | In each of these cases, the abstraction / invariant required these
> > | assignments.
> > |
> > | Recommendation: restore these assignments.
> > |
> > | Fallback recommendation: in place of these assignments, put a warning
> > | comment to the effect that the invariant is no longer maintained.
> > |
> > | The code works fine without these assignments.  But if you add more
> > | code to one of these routines, the broken abstraction may cause the
> > | code fail.  Formally: each assignment was required to maintain an
> > | invariant.
> > |
> > | Here's the tradeoff:
> > |
> > | With assignments
> > |
> > | - lint-like programs complain about a useless assignment (that's why I
> > |   previously added an explanatory comment to each of these assignments)
> > |
> > | Without assignment
> > |
> > | - code modified in the obvious way will fail
> > |
> > |
> > | Invariants:
> > |
> > | c1f6a2bf6041abb5df918fcfd3fa118bb7c761c7:
> > |
> > |     "sep" indicates what string needs to precede the next chunk
> > |     (if any) to be added to the log line.
> > |
> > | c1f6a2bf6041abb5df918fcfd3fa118bb7c761c7
> > |
> > |     next_byte is the index of the next byte of keying material to
> > |     use.
> > |
> > | ce10f9bbaa4f3e0c1926e1a87c75d336b922aa38
> > |
> > |     loggedit ("logged it", not "log edit") indicates whether
> > |     demandbuf has been logged.
> > |
> > | If an invaraint is not obvious to the reader (I think that it is),
> > | then a comment describing the invariant should be added at the
> > | variable's definition.
> >
> >
> > | From: Paul Wouters <paul at nohats.ca>
> > | Date: Thu, 6 Dec 2018 08:43:43 -0500 (EST)
> > |
> > | On Wed, 5 Dec 2018, D. Hugh Redelmeier wrote:
> > |
> > | > In each of these cases, the abstraction / invariant required these
> > | > assignments.
> > | >
> > | > Recommendation: restore these assignments.
> > |
> > | If people add things in a chain without reading through the chain to
> > | realise this, we are lost already?
> > |
> > | > - lint-like programs complain about a useless assignment (that's why I
> > | >  previously added an explanatory comment to each of these assignments)
> > |
> > | That didn't prevent coverity from complaining, which was the reason I
> > | removed them.
> > |
> > | Note the loggedit was particularly useless, as it was a chained if else
> > | loop so even extending it would have made the whole thing useless.
> > |
> > | So my preference is to not restore these.
> > _______________________________________________
> > 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