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

From: Andrey Semashev (andrey.semashev_at_[hidden])
Date: 2019-12-05 10:18:46

On 2019-12-05 12:45, Alexander Grund via Boost wrote:
> 
>> I'm strongly opposed to runtime checks regardless of the error outcome 
>> in release mode (ok, when NDEBUG is defined) because most of the time 
>> you already know the index is within bounds. When you don't know, you 
>> most likely should have check that earlier (e.g. before the parsing 
>> loop or before your higher level operation starts).
> 
> That was always my counter argument: If you can prove it to a reviewer 
> looking at your code that the index is in-bounds, then the compiler can 
> do the same and remove the check and exception.

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.

> 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[].

>> I'm opposed to throwing an exception in case of failure because the 
>> exception and its error description is likely meaningless to high 
>> level code and nearly useless when debugging. The higher level code 
>> may not want an exception to begin with, in which case it would 
>> perform the check itself, and the check in operator[] becomes useless.
> 
> No, the check gets optimized away if you do everything right.

As I said above, you can't count on that.

> If you 
> don't (do it right and have no check/exception/abort) you'll get UB, 
> buffer overflows and security risks. Especially for a stack entity this 
> will pretty much always be a SERIOUS security issue.
> So to protect users one would need at least std::abort, but using an 
> exception allows to recover from it.

You can't recover from an unexpected exception. And it is unexpected by 
account that we're talking about a human mistake.

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.