0

Being new to C++, I am still struggling with pointers-to-pointers and I am not sure if my method below is returning decoded image bytes properly.

This method gets a base64 encoded image string from API. The method has to follow this signature as it is part of legacy code that is not allowed to abbreviate from the way it was written originally. So the signature has to stay the same. Also, I have omitted here async calls and continuations, exceptions etc for code simplicity.

int __declspec(dllexport) GetInfoAndPicture(CString uid, char **image, long *imageSize)
{
    CString request = "";
    request.Format(url); 

    http_client httpClient(url);
    http_request msg(methods::POST);

    ...

    http_response httpResponse;
    httpResponse = httpClient.request(msg).get();  //blocking
    web::json::value jsonValue = httpResponse.extract_json().get();

    if (jsonValue.has_string_field(L"img"))
    {
        web::json::value base64EncodedImageValue = jsonValue.at(L"img");
        utility::string_t imageString = base64EncodedImageValue.as_string();  
        std::vector<unsigned char> imageBytes = utility::conversions::from_base64(imageString);
        image = (char**)&imageBytes;  //Is this the way to pass image bytes back? 
    *imageSize = imageBytes.size();
    }

    ...
}

The caller calls this method like so:

char mUid[64];
char* mImage;
long mImageSize;
...
resultCode = GetInfoAndPicture(mUid, &mImage, &mImageSize);

//process image given its data and its size

I know what pointer to pointer is, my question is specific to this line

image = (char**)&imageBytes;

Is this the correct way to return the image decoded from base64 into the calling code via the char** image formal parameter given the above method signature and method call?

I do get error "Program .... File: minkernel\crts\ucrt\src\appcrt\convert\isctype.cpp ... "Expression c >= -1 && c <= 255"" which I believe is related to the fact that this line is not correctly passing data back.

MikeCAT
  • 73,922
  • 11
  • 45
  • 70
pixel
  • 9,653
  • 16
  • 82
  • 149
  • imageBytes is a local variable. Even if that cast was correct (it isn't) then the variable is still gone after the subroutine exits. – stark Oct 02 '20 at 18:20
  • Do yo have the freedom to change the itnerface of `GetInfoAndPicture`? I understand that you are responsible for its implementation. – R Sahu Oct 02 '20 at 18:22

2 Answers2

1

Give the requirements there isn't any way to avoid allocating more memory and copying the bytes. You cannot use the vector directly because that is local to the GetInfoAndPicture function and will be destroyed when that function exits.

If I understand the API correctly then this is what you need to do

//*image = new char[imageBytes.size()];  //use this if caller calls delete[] to deallocate memory
*image = (char*)malloc(imageBytes.size());  //use this if caller calls free(image) to deallocate memory
std::copy(imageBytes.begin(), imageBytes.end(), *image);
*imageSize = imageBytes.size();

Maybe there is some way in your utility::conversions functions of decoding directly to a character array instead of to a vector, but only you would know about that.

john
  • 85,011
  • 4
  • 57
  • 81
  • Thank you john. I am getting now following errors "Call to std::copy' with parameters that may be unsafe" and "= cannot convert from unsighed char to char *" on the line that calls std::copy? I dont have a way to decode to char array directly, ony to the type I used above. – pixel Oct 02 '20 at 19:02
  • 1
    @pixel Sorry my mistake, `std::copy(imageBytes.begin(), imageBytes.end(), *image);` missed `*` before `image`. You could also use `memcpy` to do the same job, if you are more familar with that. The unsafe parameters message is just a warning. The MS compiler is very sensitive to anything that might cause a buffer overrun, the code above allocates enough room. – john Oct 02 '20 at 19:12
  • thanks again. That removed 2nd error but the first one is still present and it is not warning but error "Error C4996 'std::copy::_Unchecked_iterators::_Deprecate': Call to 'std::copy' with parameters that may be unsafe - this call relies on the caller to check that the passed values are correct.". And no, I am not familiar with memcopy and C or C++ in general. Very new to these languages. – pixel Oct 02 '20 at 19:23
  • Thanks John. REgarding that error, it seam to be a warning but it shows like error in VS2017 and faild compilation. But I figured out that you can r-c on project > Properties > C/C++ > SDL checks and set it to No. After this, I was able to build as you suggested. Much appreciated! – pixel Oct 02 '20 at 20:15
  • 1
    @pixel OK glad it's working. As you're new to C++ I should just mention that the code I gave leaks memory. At some point you do need to `delete[]` the image data. – john Oct 03 '20 at 06:24
  • Thanks for letting me know. In my case, the caller of GetInfoAndPicture calls free(image) in its destructor so it is clearing memory. The only change I made to your suggestion for that reason is not to call *image = new char[imageBytes.size()] but to use instead *image = (char*) malloc(imageByte.size()); I added this detail to your answer I accepted earlier to help others. If you dont mind just checking that would be greatly appreciated and thank you. – pixel Oct 03 '20 at 22:33
  • 1
    @pixel Seems reasonable, if the caller uses `free` then `malloc` must be used here. I just changed `deallocate()` to `delete[]` in the comment. – john Oct 04 '20 at 05:36
0

The problem is with allocating (and freeing) memory for that image; who is responsible for that?

You can't (shouldn't) allocate memory in one module and free it in another.

Your two options are:

  1. Allocate large enough buffer on the caller side, and have DLL use it utility::conversions::from_base64(). The issue here is: what is large enough? Some Win APIs provide an additional method to query the required size. Doesn't fit this scenario as the DLL would either have to get that image for the second time, or hold it (indefinitely) until you ask for it.
  2. Allocate required buffer in the DLL and return a pointer to it. You need to ensure that it won't be freed until the caller request to free it (in a separate API).
Vlad Feinstein
  • 10,960
  • 1
  • 12
  • 27