Re: [review][fixed_string] Andrzej's feedback

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2019-12-05 12:26:54

czw., 5 gru 2019 o 12:23 Alexander Grund via Boost <boost_at_[hidden]>
napisał(a):

> > No, you're giving too much credit to compilers. Compilers cannot
> > analyze at the same level as humans do. For example, if the valid
> > index is stored in a data member in one method of a class and used in
> > another, and the first method is required to be called first (which
> > might also be enforced by a runtime check via another member
> > variable), the compiler is not able to know that the index is always
> > valid. At least, I haven't seen such cleverness.
>
> That's why there is usually a checked and an unchecked method. Your
> usual accesses though are: `for(auto c: string) ...`, for(int i=0;
> i<string.size();i++) do(string[i]);`, `if(i+1<string.size())
> bar(string[i], string[i+1])` and the like. Easy on optimizers
>
> The scenario you describe is hard to check for a human to: Are you sure
> this invariant always holds if it is enforced in some other method which
> need to have been called before? For all iterations of code evolving? If
> so then better use the string.unsafe_get_i_have_checked_the_index(i)
> method (made-up). This is a clear signal to any reviewer/reader that the
> index was indeed checked and the unsafe call is deliberately used.
>
> >> I never bought "most of the time you already know the index is within
> >> bounds" because it is equivalent to "most of the time your code is
> >> correct, let's not use checks/unit tests/hardening/..."
> >> If what you say is true, then buffer overflows would be non-existant.
> >> Are they?
> >
> > Mistakes happen, I'm not arguing with that. But defensive programming
> > is not the solution because it doesn't save you from these mistakes.
> > Code that passes invalid index to at() is just as incorrect as the one
> > that does so with operator[].
> No. Defensive programming is a safety net. If you always use at() and
> only in specific scenarios [] then at least your application will crash
> with a possibly good stacktrace or you can catch the exception in
> main(), prepare a meaningful error, log it, send it to your server and
> play jingle bells.
> >> No, the check gets optimized away if you do everything right.
> >
> > As I said above, you can't count on that.
> So what? Marginally lower performance if even measurable but safe from
> buffer overflows.
> > You can't recover from an unexpected exception. And it is unexpected
> > by account that we're talking about a human mistake.
> You can: For e.g. a connection/parser/... you terminate the connection
> and log the issue. At the least you can catch it in main or some
> exception hook to log a backtrace and exit gracefully.
> > The correct way of tackling mistakes is human reviewing, testing and
> > debugging. For the debugging part, a crash with a saved backtrace is
> > more useful than an exception with a meaningless error message like
> > "fixed_string::at", as many standard libraries like to do.
>
> You don't get a crash, at least not with a "meaningful backtrace" if you
> don't use exceptions. You get UB, a security issue and possibly a crash
> in some other part later instead of where the error was detected. And
> why would "human reviewing, testing and debugging" be more reliable than
> "human codewriting"? It is still a human and humans make mistakes.
> Better use a safety net.
>

I think that this discussion diverges from the point of this thread. It was
not about operator[] (for which it is clear that passing a too big index is
a bug) but about resizing over fixed_string's capacity, in which case the
library doesn't make it clear if it is even a bug, or something that the
programmer may want to do.

For instance, in std::sting it is not a bug to resize over the capacity,
and fixed_string is supposed to be a drop-in replacement.

I would like to make this question sorted out first; and only then can we
decide what to put inside the function.

Regards,
&rzej;