CODEWITHSUNDEEP
c++pointersmemory-management
mreff555
mreff555

Reputation: 1154

Unable to allocate memory via pointer

I wrote this function, in which the intention is to combine the character equivalent of argument 3, with argument 2. Then allocate memory for argument 1 and return it. Based on debug statements inserted into the function everything seems to be correct, but it appears to be freeing the memory on return. Why is this? or am I missing something else?

I'm not accustomed to programming on a mac and I can't get gdb to work, so I'm kinda flying blind.

Function

bool BraviaIpCtrl::setVolume(char *output, const char *input, unsigned short value)
{
  bool success = false;
  output = nullptr;

  if(value <= 100)
  {
    int msgLen = 24;
    output = new char[msgLen];
    memset(output, 0, sizeof(*output));
    std::string numbers = std::to_string(value).c_str();
    size_t len = numbers.length();
    memcpy(output, input, msgLen);
    memcpy(output + (msgLen - 1) - len, numbers.c_str(), len);
    success = true;
  }
  return success;
}

Test Function call

  char* test = nullptr;

  if(bc.setVolume(test, bc.bctl_volume_set, 43) && test != nullptr)
  {
    std::cout << *test << std::endl;
  }
  else
  {
    std::cout << "NOPE!!" << std::endl;
  }

Upvotes: 0

Views: 73

Answers (2)

Giacomo Pirinoli
Giacomo Pirinoli

Reputation: 568

As @mailtreyak pointed out, you are passing a pointer to char:

  • a copy of pointer output (let's say output_copy) is used within the function,
  • if you make output_copy point to some different data/memory, your output pointer is still pointing to its previous data/memory,
  • as soon as you exit the function the modification you expect has not happened (but this is correct, because the data/memory output is pointing to have not been modified at all).

Here below you can find another way using PointerToPointer (**): 

bool BraviaIpCtrl::setVolume(char** output, const char* input, unsigned short value)
{
    bool success = false;
    *output = nullptr;

    if (value <= 100)
    {
        int msgLen = 24;
        *output = new char[msgLen];
        memset(*output, 0, msgLen);
        std::string numbers(*output);
        size_t len = numbers.length();
        memcpy(*output, input, msgLen);
        memcpy(*output + (msgLen - 1) - len, numbers.c_str(), len);
        success = true;
    }
    return success;
}

and calling code:

char* test = nullptr;
if (bc.setVolume(&test, bc.bctl_volume_set, 43) && test != nullptr)
{
    std::cout << *test << std::endl;
}
else
{
    std::cout << "NOPE!!" << std::endl;
}

Please pay much attention to this error in your previous code:

int msgLen = 24;
output = new char[msgLen];
memset(output, 0, sizeof(*output));

should be instead:

int msgLen = 24;
output = new char[msgLen];
memset(output, 0, msgLen);

this because you want to set 24 bytes, not just only 1 byte (1 = sizeof(*output), the size of a pointer to char)

Upvotes: 1

maitreyak
maitreyak

Reputation: 269

The problem is that you are passing the pointer variable to the function and like any other variable it is passed by value, so the method "setVolume" is making a local copy of pointer test and assigning memory. The calling test method has no way of seeing this change.

Why not change the method implementation to return the address of the array instead.

char * BraviaIpCtrl::setVolume(const char *input, unsigned short value)
{
  char*  output = NULL;
  if(value <= 100)
  {
    int msgLen = 24;
    output = new char[msgLen];
    memset(output, 0, sizeof(*output));
    std::string numbers = std::to_string(value).c_str();
    size_t len = numbers.length();
    memcpy(output, input, msgLen);
    memcpy(output + (msgLen - 1) - len, numbers.c_str(), len);
  }
  return output;
}

Upvotes: 1

Related Questions