[squid-dev] [PATCH] SBuf case-change operations are safe against OutOfBoundsEception

[Alex Rousskov]

On 06/27/2015 12:56 PM, Kinkie wrote:

>   I'd like some feedback on this patch. Coverity detects an uncaught
> exception code path for OutOfBoundsException which eventually leads to
> SBuf.toUpper. However in single-threaded mode the exception cannot be
> thrown by the loop calling that code.


> Is it reasonable to explicitly catch that exception as I do in the
> attached patch 

Catching an exception that [the reason tells you] cannot be thrown is
not reasonable.


> or should we rather triage and mark as false positives in
> coverity all uses of this method?

It is a false positive AFAICT. If you agree, then marking it as such
would be the second best option IMHO.


> Or is there a more effective way to obtain the same result?

I hope so: SBuf::toUpper() and toLower() methods should use a [private]
character setting method that does not check/throw anything. If there is
no such method, add it. You may call it riskySetAt(), justSetAt(), or
setAtUnchecked().

Moreover, calling cow() on every update operation also adds a little
overhead, even though only the first call can actually do something
useful. There are several ways to optimize that. Here is a sketch that
also eliminates code duplication between toUpper() and toLower() while
exposing an optimized method potentially useful to others:

template <class UnaryOperation>
void SBuf::transform(UnaryOperation op) {
    size_type updates = 0;
    for (size_type pos = 0; pos < length(); ++pos) {
        const int original = (*this)[pos];
        const int updated = op(original);
        if (original != updated) {
            if (!updates++)
                cow();
            setAtUnchecked(pos, updated);
        }
    }
}

You may use lambdas to implement SBuf::toUpper() and toLower() via
transform(). The standard toupper(3) and tolower(3) are locale-dependent
so we probably should not be using them for protocol work (for overhead
reasons; even if we force the locale to "C"). BTW, should not we use our
xtoupper() and xtolower() instead of toupper(3) and tolower(3)?


Alternatively, you may add a similar cow-avoiding trick directly to
SBuf::toUpper() and toLower() code, continuing to duplicate it.


HTH,

Alex.