Re: [lmi] [PATCH] Remove unnecessary definitions of pure virtual functions

[Vadim Zeitlin]

On Wed, 15 Mar 2017 19:21:31 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2017-03-15 16:51, Vadim Zeitlin wrote:
...
GC> >  IMO the most preferable alternative by far would be to forbid 
constructing
GC> > the object in an invalid state, IME such objects are a huge source of
GC> > problems (if NULL was Hoare's billion-dollar mistake, then invalid objects
GC> > are at least a 500-million-dollar one).
...
GC> I would nevertheless maintain that class round_to, as it stands,

 This is a perfectly acceptable pragmatic solution, there is no urgent need
to go change all lmi code using this class right now and there is
definitely no security vulnerability here. But I still think the idea of
allowing objects to (implicitly) be in an invalid state is a poor one and
I'd like to recommend avoiding it in any new code.

GC> chooses the option that is least bad, or at least no worse than any
GC> other, if we quite reasonably want to make this class
GC> default-constructible--e.g., so that we can do
GC>   std::vector<round_to> RoundFns;

 It's not obvious to me that we'd ever need to have a vector of these
objects, is this really a realistic example?

GC> or even so that we can just compile lmi as it stands, given:
GC> 
GC>   class LMI_SO BasicValues{...
GC>     round_to<double> round_specamt_;
GC>   ...};
GC> 
GC>   void BasicValues::SetRoundingFunctors()
GC>   {...
GC>     set_rounding_rule(round_specamt_, 
RoundingRules_->datum("RoundSpecAmt"));
GC>   ...}

 This is a very nice example though because it shows that accepting the
possibility of invalid state in round_to<> practically inevitably leads to
also allowing a BasicValues object to be in an invalid state as well,
whereas not allowing the invalid state of round_to<> would discourage
allowing BasicValues to be in an invalid state too, while still allowing it
if really needed.


GC> Should we give up the ability to hold a round_to object as a member? or the
GC> ability to initialize it after the proper context has been set (such as the
GC> name of the file from which the rules are read)?

 Again, ideal would be to forbid creating BasicValues in an invalid state
and provide a factory function taking the name of the file, reading the
rules from it and then creating BasicValues with the rounding method
specified by these rules.

 If this is really impossible, we could always use unique_ptr<round_to<>>
or, ideally, std::optional<round_to<>>, instead of the object itself. But
IME it's rarely impossible and trying to avoid the invalid states naturally
results in better code architecture and is, of course, more robust.

GC> We could initialize rounding_function_ to a "common" choice by default:

 No, this is clearly a bad idea, let's not even consider it.

GC> Alternatively, if we want to do something like
GC>   std::vector<round_to>(9)
GC> but we aren't willing to make the class DefaultConstructible, then of course
GC> we could resort to
GC>   std::vector<std::shared_ptr<round_to>>

 I don't think there is any call for the use of shared_ptr<> here, a
unique_ptr<> (or, again, std::optional<> when it's available), would be
quite enough.


GC> Consider again the BasicValues code above. If (as a matter of corporate
GC> policy, say) we had to conform to every securecoding.cert.org rule, then
GC> lmi would be deemed "insecure", and we'd be compelled to fix it...and we
GC> would naturally fix this "violation" by eradicating an appropriate use of
GC> "[[noreturn]]", even though that would make lmi no better and obviously
GC> no more "secure". As a "suggestion to consider" or a "guideline to follow
GC> unless there's a good reason not to", it's okay. But they mandate what we
GC> "must" do, raise the specter of "undefined behavior", speak of "Noncompliant
GC> Code", present a "Risk Assessment", and call this a "vulnerability"; that's
GC> uncalled for and not even correct, and it weakens the credibility of the
GC> legitimate, reasonable guidance they ordinarily publish.

 Yes, I broadly agree with this, but I also see the rationale for this
guideline: using [[noreturn]] for non-void functions may indicate confusion
about what exactly does this attribute do and I strongly suspect that there
are more erroneous uses of it in this case than correct ones. But it's
clearly not a security vulnerability, of course.

 Regards,
VZ