1

I have this function which simply encrypts a string (this function works fine, and tested).

DWORD SomeObj::Encrypt(string * To_Enc) {
    DWORD text_len = (To_Enc->length());
    if (!CryptEncrypt(this->hKey,
        NULL,  // hHash = no hash
        1,  // Final
        0,     // dwFlags
       (PBYTE)(*To_Enc).c_str(), //*pbData
       &text_len,  //*pdwDataLen
       128)) {      //dwBufLen
       return SERVER_ERROR;
    }
    return SERVER_SUCCESS;
}

And I have this piece of code:

string s= "stringTest";

Encrypt(&s);

which simply call the function passing the pointer of a string.

The program is causing an access violation exception right when it calls the function Encrypt(&s), I guess that it's something about the parameter &s being passed but I can't figure this out. Any idea from your experience ?

Vishaal Shankar
  • 1,648
  • 14
  • 26
rafiki
  • 91
  • 1
  • 8
  • You are not allowed to change the contents of a `std::string` via the pointer returned from `std::string::c_str()` – Richard Critten Mar 15 '18 at 12:13
  • You're also not providing 128 bytes of buffer. – molbdnilo Mar 15 '18 at 12:14
  • that is absolutely not true, I have successfully did it an hundred times... @RichardCritten – rafiki Mar 15 '18 at 12:19
  • the string suppose to grow on demand. the problem is not about this, it doesnt even get to this point. this very same function, and call to the function, works perfectly on another VS project. @molbdnilo – rafiki Mar 15 '18 at 12:20
  • 2
    _"...Writing to the character array accessed through c_str() is undefined behavior...."_ source: http://en.cppreference.com/w/cpp/string/basic_string/c_str UB includes appearing to work by accident. – Richard Critten Mar 15 '18 at 12:29
  • 1
    @LAvR You can't extend a `std::string` by writing to its underlying storage. If it worked in a different project, you were just having bad luck. – molbdnilo Mar 15 '18 at 12:43
  • That's how the CryptEncrypt work, by overwiriting the old value of the string. I doubt it is only a bad luck, it works every time I run it ... @molbdnilo – rafiki Mar 15 '18 at 12:46
  • @LAvR But your old value of the string isn't 128 bytes long, and writing past its last character is undefined. That a C++ program appears to work does not imply that it's not incorrect. – molbdnilo Mar 15 '18 at 13:11
  • Tnx for your time, I understand, so what are suggest ? @molbdnilo – rafiki Mar 15 '18 at 13:50

1 Answers1

0

This answer will reiterate important points already made in the comments, with example code.

Your current code:

DWORD SomeObj::Encrypt(string * To_Enc) {
    DWORD text_len = (To_Enc->length());
    if (!CryptEncrypt(this->hKey,
        NULL,  // hHash = no hash
        1,  // Final
        0,     // dwFlags
       (PBYTE)(*To_Enc).c_str(), //*pbData
       &text_len,  //*pdwDataLen
       128)) {      //dwBufLen
       return SERVER_ERROR;
    }
    return SERVER_SUCCESS;
}

On the line:

(PBYTE)(*To_Enc).c_str(), //*pbData

Note that you are casting away const-ness from the pointer value returned from the c_str() method call.

This should immediately be a red flag; there may be times when casting away const-ness is a valid use case, but it is more the exception than the rule.

Untested, but using a temporary, mutable buffer should solve your problem, such as:

#include <cstddef>
#include <vector>
...
DWORD SomeObj::Encrypt(string * To_Enc) {
    std::vector<std::string::value_type> vecBuffer(To_Enc->length() * 3, 0);  // use the maximum value that could be output, possibly some multiple of the length of 'To_Enc'
    std::size_t nIndex = 0; 
    for (auto it = To_Enc->cbegin(); it != To_End->cend(); ++it)
    {
        vecBuffer[nIndex++] = *it;
    }
    DWORD text_len = (To_Enc->length());
    if (!CryptEncrypt(this->hKey,
        NULL,  // hHash = no hash
        1,  // Final
        0,     // dwFlags
       reinterpret_cast<PBYTE>(&vecBuffer[0]), //*pbData
       &text_len,  //*pdwDataLen
       vecBuffer.size())) {      //dwBufLen
       return SERVER_ERROR;
    }
    To_Enc->assign(&vecBuffer[0], text_len);  // assumes 'text_len' is returned with the new length of the buffer
    return SERVER_SUCCESS;
}
Phil Brubaker
  • 1,257
  • 3
  • 11
  • 14