How to protect a value of a local variable after leaving scope?

Question

I wrapped a C library's subscribe function into a higher level C++ class' method. Inside that method the underlying C subscribe is called with a local char* variable (char key[10]) passed in as reference. The problem is now - which I have just figured out - since key is a local variable its value is not protected. I can pass its reference but that memory will be free once the scope is left. I am experiencing this undefined behaviour by the callback is never called - after debugging I saw that the value of the key is altered.

I have tried using new char[10] which seemed to work. But I guess it's not the way I supposed to go to.

What is the correct solution for this? Update: it is now fixed by replacing with string.

Update

Interface function:

IoT_Error_t aws_iot_mqtt_subscribe(AWS_IoT_Client *pClient, const char *pTopicName, uint16_t topicNameLen,
                               QoS qos, pApplicationHandler_t pApplicationHandler, void *pApplicationHandlerData)

Wrapper:

    std::function<void()> AWS::subscribe(const std::string &topic, std::function<void(const std::string&)> callback, QoS qos) {
  ESP_LOGI(TAG, "subscribe: %s", topic.c_str());

  std::string key("Test...");

  auto task = c_style_callback(
    [=] (AWS_IoT_Client *pClient, char *topicName, uint16_t topicNameLen, IoT_Publish_Message_Params *params) {
      std::string json;
      json.assign((char *)params->payload, (size_t)params->payloadLen);
      ESP_LOGI(TAG, "subscribe cb payload=%s", json.c_str()); // works
      ESP_LOGI(TAG, "key '%s'", key.c_str()); // undefined behaviour
      callback(key); // error, exit
      callback(json);
    }
  );

  m_error = ::aws_iot_mqtt_subscribe(
    &m_client,
    key.c_str(),
    key.length(),
    qos,
    task.get_callback<AWS_IoT_Client*, char*, uint16_t, IoT_Publish_Message_Params*>(),
    task.get_pvoid()
  );

  if (m_error != SUCCESS) {
    ESP_LOGD(TAG, "subscribe: error=%d", m_error);
    return nullptr;
  }

  return [=] () {
    ESP_LOGI(TAG, "unsubscribe %s", key.c_str());  // works
    callback(key); // works
  };
} // subscribe

c_style_callback utility function:

template<class F>
struct c_style_callback_t {
  F f;
  template<class...Args>
  static void(*get_callback())(Args..., void*) {
    return [](Args...args, void* fptr)->void {
      (*static_cast<F*>(fptr))(std::forward<Args>(args)...);
    };
  }
  void* get_pvoid() {
    return std::addressof(f);
  }
};

template<class F>
c_style_callback_t< std::decay_t<F> >
c_style_callback( F&& f ) { return {std::forward<F>(f)}; }

Main task where the subscribe wrapper is being called:

{
...
aws->subscribe(
  topic,
  [&] (const std::string &json) -> void {
    ESP_LOGI(TAG, "got json: %s", json.c_str());
  }
);
...
}

Update #2

The callback lambda inside the c_style_callback does not access to the correct values of callback and key. How to protect these 2 from getting overwritten? "Wrapping" them inside a unique_ptr? Return task to the caller for reference? Also, the return value of the helper get_pvoid() points the user data which is the lambda function, maybe that should be protected?

In C++ you do this by passing in `const` data. Please don't tag as C and C++ if you want a C++ answer. — tadman, Sep 13 '18 at 18:44
Most likely `char key[10]` is fine. Check the documentation on the `subscribe` function to see what kind of pointer it expects. — NathanOliver, Sep 13 '18 at 18:45
please post the interface function's declaration and a sample of code with which you're trying to call it. — Richard Hodges, Sep 13 '18 at 18:46
As a rule of thumb, for situations like this, `std::string` (or string_view) is usually your first choice. — Jerry Coffin, Sep 13 '18 at 18:49
@JerryCoffin string_view only useful is part of the c interface contains a size parameter. string_views don't always have a trailing zero. — Richard Hodges, Sep 13 '18 at 18:53
@RichardHodges: While more or less true, I'm not sure how that's particularly relevant. I haven't looked through all the code that's been added, but at least as originally posted, I don't recall seeing enough information to be sure whether a string_view would work or not. — Jerry Coffin, Sep 13 '18 at 19:03
One note on your code: Whenever you are using array new, you are very surely doing something wrong. Use `std::vector` instead. Even using plain new is usually a sign of bad code. — Ulrich Eckhardt, Sep 13 '18 at 19:14

score 2 · Accepted Answer · answered Sep 13 '18 at 18:53

2

The way to do this is to allocate the memory on the heap (new, malloc). You have several choices:

Preferred choice: If you new the memory, you can wrap the pointer into a std::unique_ptr or std::shared_ptr, and safely pass that around. When the last instance goes out of scope the memory is automatically released. Direct access to the pointer can be gained with get(). You must ensure that noone else releases your memory!
Use a RAII(Resource Acquisition Is Initialization)-wrapper type, which does malloc and free, or new and delete. In this case you allocate the memory via the constructor, and release it via the destructor, while you pass the raw pointer to the C-interface. Has the downside that you have to implement copy/move-semantics to make sure you don't release twice, and that you correctly keep track of all copies that store this pointer. Due to the complexity this involves, I would advise you use unique/shared_ptr, if you can by any means. It is also possible to pass a custom deleter to the shared_ptr, so you can also use it with free as a deleter.
You could use the raw pointer from new, but the you have to ensure to delete it exactly once.

answered Sep 13 '18 at 18:53

midor

5,487
2
23
52

1

Naked `new`/`delete` should be avoided. Maybe mention `std::make_unique`/`std::make_shared`.. – Jesper Juhl Sep 13 '18 at 19:12
@midor Which one you think is best for this problem? `std::unique_ptr` or `std::shared_ptr`? Can you please add some simple code of using one of these? – haxpanel Sep 13 '18 at 19:23
1

@haxpanel General rule of thumb: Start with the most restrictive, `std::unique_ptr`, and weaken the restrictions, `std::shared_ptr` in this case, only if forced to. The hint is in the name: `shared_ptr` should be used if the object is shared. If multiple other objects have an ownership stake in the object--the object must not be destroyed so long as any of the holders are using it--only then should you use a `shared_ptr`. [A few words worth reading on the concept of Ownership](https://stackoverflow.com/questions/49024982/what-is-ownership-of-resources-or-pointers). – user4581301 Sep 13 '18 at 19:46
if you have to make the choice as a function return value, I would go with the `unique_ptr`, because that can be implicitly converted to a `shared_ptr`. – midor Sep 16 '18 at 20:04

How to protect a value of a local variable after leaving scope?

1 Answers1