C4996 warning due to std::reverse_copy on Microsoft platforms

[noloader]

We recently cut-in little-endian Integers. On Microsoft platforms, use of _std:reverse_copy_ is causing a C4996 warning:

1>c:\Program Files\...\VC\include\algorithm(2184): warning C4996: 'std::_Reverse_copy': Function call with parameters that may be unsafe - this call relies on the caller to check that the passed values are correct. To disable this warning, use -D_SCL_SECURE_NO_WARNINGS. See documentation on how to use Visual C++ 'Checked Iterators'
1>          c:\...\VC\include\algorithm(2168) : see declaration of 'std::_Reverse_copy'
1>          integer.cpp(2898) : see reference to function template instantiation '_OutIt std::reverse_copy<const byte*,unsigned char*>(_BidIt,_BidIt,_OutIt)' being compiled
1>          with
1>          [
1>              _OutIt=unsigned char *,
1>              _BidIt=const byte *
1>          ]

The offending code is:

Integer::Integer(const byte *encodedInteger, size_t byteCount, Signedness s, ByteOrder o)
{
    if(o == LITTLE_ENDIAN_ORDER)
    {
        SecByteBlock block(byteCount);
        std::reverse_copy(encodedInteger, encodedInteger+byteCount, block.begin());

        Decode(block.begin(), block.size(), s);
        return;
    }
    ...
}

I think we need to find a Microsoft overload that allows us to specify the destination buffer size. Or we need to use the library's ByteReverse from misc.h:

template <class T>
    void ByteReverse(T *out, const T *in, size_t byteCount);

[DevJPM]

@noloader I don't think ByteReverse<> is the right choice here.

ByteReverse<> iterates over all the objects of type T and individually byte reverses them.

So {1,2,3,4} would still be {1,2,3,4}.

std::reverse_copy OTOH takes your list and reverses it, e.g. it makes {1,2,3,4} to {4,3,2,1}, which is what we need here.

Luckily Microsoft already proposed a fix on MSDN.

If you look at the second code sample, in the last line. stdext::checked_array_iterator apparently can be used to fix the warning. All we have to do would be to replace the block.begin() by stdext::checked_array_iterator<byte*>(block.begin(),byteCount)

---

However, it appears to be the case that GCC (and other compilers) may not support stdext::checked_array_iterator<>, for this case we have to use an #ifdef _MSC_VER stdext::checked_array_iterator<> #else // do a manual check here #endif.
Also see "checked_array_iterator in C++11 on SO"

[noloader]

However, it appears to be the case that GCC (and other compilers) may not support...

Yeah, we should probably split things here (as much as I dislike doing it):

// I believe 14.10 is the version we need/want. I believe that's
//   when Microsoft cut-in the safer functions.
#if (CRYPTOPP_MSC_VERSION >= 1410)
    std::reverse_copy(encodedInteger, encodedInteger+byteCount, 
        stdext::make_checked_array_iterator(block.begin(), block.size()));
#else
    std::reverse_copy(encodedInteger, encodedInteger+byteCount, block.begin());
#endif

The reason it should be split is because its a Certification and Accreditation (C&A) issue on Windows platforms. That is, compliance requires some users to follow Microsoft's best practices on Windows platforms. There is no wiggle room.

Do you feel like making a pull request?

Don't worry if we are off-by-one or so on the MSVC compiler version. Most users won't experience it; and we'll be able to tune it later when I test on Windows platforms. Though I cannot test VS2015, I can test back to VC++ 6.0 and Visual Studio .Net.

[noloader]

How about a plain old reverse:

$ cat git.diff 
diff --git a/integer.cpp b/integer.cpp
index 3a67010..2cdc201 100644
--- a/integer.cpp
+++ b/integer.cpp
@@ -2894,8 +2894,8 @@ Integer::Integer(const byte *encodedInteger, size_t byteCount, Signedness s, Byt

    if(o == LITTLE_ENDIAN_ORDER)
    {
-       SecByteBlock block(byteCount);
-       std::reverse_copy(encodedInteger, encodedInteger+byteCount, block.begin());
+       SecByteBlock block(encodedInteger, byteCount);
+       std::reverse(block.begin(), block.begin()+block.size());

        Decode(block.begin(), block.size(), s);
        return;

The downside to the code above is each element is touched twice. The first copies it into the SecByteBlock, the second reverse it in the SecByteBlock. But we don't need to worry about complying with Microsoft C&A items.

[noloader]

// I believe 14.10 is the version we need/want. I believe that's
//   when Microsoft cut-in the safer functions.
#if (CRYPTOPP_MSC_VERSION >= 1410)

It looks like checked iterators were introduced in VS2008. I think the test should be:

#if (CRYPTOPP_MSC_VERSION >= 1500)
    std::reverse_copy(encodedInteger, encodedInteger+byteCount, 
        stdext::make_checked_array_iterator(block.begin(), block.size()));
#else
    std::reverse_copy(encodedInteger, encodedInteger+byteCount, block.begin());
#endif

In the absence of a patch, I'm going to use the MSC guard so that folks enduring an audit won't encounter trouble.

[noloader]

Also see Commit f45813bd129c1d58.

[JanFellner]

I wanted to link this pretty old issue with a new one that arises out of the changes beeing made
#1250
VS 2022 17.8 seems not to really love the make_checked_array_iterator
As i have no deeper understanding about the changes made here and how to test them...
Maybe one of the authors of this change can take a look.