6

I have a template function with a specialization that performs zeroization:

template <class T>
void SecureWipeBuffer(T *buf, size_t n)
{
    volatile T *p = buf+n;
    while (n--)
        *((volatile T*)(--p)) = 0;
}
...

template <>
void SecureWipeBuffer(word64* p, size_t n)
{
   asm volatile("rep stosq" : "+c"(n), "+D"(p) : "a"(0) : "memory");
}

Coverity is producing a finding on SecureWipeBuffer:

word64 val;
...
SecureWipeBuffer(&val, 1);

The finding is:

>>>     CID 164713:  Incorrect expression  (SIZEOF_MISMATCH)
>>>     Passing argument "&val" of type "word64 *" and argument "1UL" to function "SecureWipeBuffer" is suspicious because "sizeof (word64)" /*8*/ is expected.
275             SecureWipeBuffer(&val, 1);

How do train Coverity that SecureWipeBuffer takes a count of elements, and not a count of bytes?


EDIT: we've picked up two similar findings with our Windows code. In addition, Coverity is producing findings on standard library functions. Its as if it does not realize C++ deals with counts of elements, and not counts of bytes.

Below is from Microsft standard library code in <xmemory>

 25    if (_Count == 0)
 26        ;
 27    else if (((size_t)(-1) / sizeof (_Ty) < _Count)
    CID 12348 (#1 of 1): Wrong sizeof argument (SIZEOF_MISMATCH)
    suspicious_sizeof: Passing argument _Count * 4U /* sizeof (std::allocator<void *>::value_type) */
    to function operator new which returns a value of type std::allocator<void *>::value_type is suspicious.
 28        || (_Ptr = ::operator new(_Count * sizeof (_Ty))) == 0)
 29            _Xbad_alloc();  // report no memory
jww
  • 97,681
  • 90
  • 411
  • 885
  • 1
    `--p` already has type `volatile T *`, so the cast is redundant – M.M Jul 15 '16 at 02:10
  • 1
    In case you are unaware, in the non-specialzed version, the standard allows for `volatile` to be optimized out and the compiler could even replace this with a memset call. (the rule about preserving access to volatile objects is just that; not accesses through volatile-qualified pointers). – M.M Jul 15 '16 at 02:13
  • 1
    @M.M - *"`--p` already has type `volatile T *`, so the cast is redundant"* - you'd think that, but two versions of GCC fail to do what's expected. Compilers doing unexpected things is the reason we have to run that loop in reverse, too. Its one of the joys of supporting 20 years of compilers on nearly every major platform. – jww Jul 15 '16 at 04:09
  • *"... in the non-specialzed version, the standard allows for volatile to be optimized out and the compiler could even replace this with a `memset` call...."* - yeah, that makes me want to cry. I wish that damn language allowed us to express ourselves properly. I'm weary of well defined programs being turned into broken programs by fiat via the AS-IF-BROKEN rule. – jww Jul 15 '16 at 04:13
  • 1
    "Compilers doing unexpected things is the reason we have to run that loop in reverse, too" - Can I suggest that you get to the bottom of why you have to do the casting and reverse iterations before you try to understand this? If your compiler isn't doing sane things when trying to compile a simple while loop, then how can you trust it to do sane things the rest of the time? – UKMonkey Oct 03 '16 at 09:09
  • @UKMonkey - Coverity produces findings for more than just the zeroizer. It generates findings in GCC and MSC standard library code, too. As a free software project, we receive a limited number of scans per week. Its hard to experiment with different things because we quickly exceed our allotment. – jww Oct 03 '16 at 16:29

1 Answers1

2

I found this Github, which tries to suppress that*, by doing this:

  std::fill_n(out, spec_.width_ - 1, fill);
  out += spec_.width_ - 1;
} else if (spec_.align_ == ALIGN_CENTER) {
  // coverity[suspicious_sizeof]
  out = writer_.fill_padding(out, spec_.width_, 1, fill);
} else {
  std::fill_n(out + 1, spec_.width_ - 1, fill);

which is also advised in Silencing false positives in Coverity Prevent, and another approach is covered here: Coverity SA - excluding boost, stlport errors.


*I am not sure if that's what you want, but that's all I got!

Community
  • 1
  • 1
gsamaras
  • 71,951
  • 46
  • 188
  • 305
  • 2
    Thanks gsamaras. I'm mulled over this for a number of weeks now (I was the upvote a few weeks back). It feels like a hack, and I think the issue should be addressed in some other way. I put a bounty on the question in hopes of getting one of the Coverity guys interested. Hopefully one of the Coverity guys will explain why analysis is treating C++ element counts as a "count of bytes"; and hopefully they will tell us how to address it without a hack (code annotations, modelling, etc). – jww Sep 29 '16 at 21:33
  • 1
    Great @jww, I am curious! :) – gsamaras Sep 30 '16 at 06:09