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 <[email protected]> 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 <[email protected]> > > | To: Libreswan Development List <[email protected]> > > | Date: Wed, 5 Dec 2018 18:06:17 -0500 (EST) > > | > > | | commit c1f6a2bf6041abb5df918fcfd3fa118bb7c761c7 > > | | Author: Paul Wouters <[email protected]> > > | | 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 <[email protected]> > > | | 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 <[email protected]> > > | | 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 <[email protected]> > > | | 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 <[email protected]> > > | 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 > > [email protected] > > https://lists.libreswan.org/mailman/listinfo/swan-dev > > > _______________________________________________ > Swan-dev mailing list > [email protected] > https://lists.libreswan.org/mailman/listinfo/swan-dev _______________________________________________ Swan-dev mailing list [email protected] https://lists.libreswan.org/mailman/listinfo/swan-dev
