Bug #3678: Face::put(const Data&) undefined behavior - ndn-cxx - NDN project issue tracking system

Actions

Copy link

Bug #3678

closed

Face::put(const Data&) undefined behavior

Added by Junxiao Shi almost 10 years ago. Updated over 9 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Junxiao Shi

Category:

Base

Target version:

v0.5

Start date:

07/19/2016

Due date:

% Done:

100%

Estimated time:

3.00 h

Description

Face::put(const Data&) recommends its data parameter to be managed by a shared_ptr to avoid copying, but does not require that.

This code checks whether a shared_ptr is available:

  try {
    dataPtr = data.shared_from_this();
  }
  catch (const bad_weak_ptr& e) {

However, until C++17, std::enable_shared_from_this::shared_from_this is undefined behavior if the Data is not previously managed by a shared_ptr.
Therefore, this checking code is unsafe in C++11 or C++14.

Actions

Copy link

Updated by Junxiao Shi almost 10 years ago

Given that this check was introduced two years ago in commit:6a05b4b0e7442f324031018596c0341432e5ac8f with a mandatory warning message, I doubt anyone is still using it.
I think the easy solution is:

require that the data must be managed by shared_ptr
delete the check and the warning message
assert that the shared_ptr<const Data> returned from shared_from_this is valid

Actions

Copy link

Updated by Davide Pesavento almost 10 years ago

Junxiao Shi wrote:

assert that the shared_ptr<const Data> returned from shared_from_this is valid

What would be the purpose of this? if the passed Data was not created via make_shared, you're already invoking undefined behavior the moment you call shared_from_this on it.

Actions

Copy link

Updated by Junxiao Shi almost 10 years ago

Reply to note-2:

Then we'll have to declare Face::put(shared_ptr<Data>). But that's going to take another two years before the old API is removed.

Actions

Copy link

Updated by Alex Afanasyev almost 10 years ago

Even though it is unsafe and undefined, it is working...

However, I have another thought. May be we can remove this completely and have copy (ideally move) put. We may also rename put to "putData" for consistency with NDN-CCL.

Actions

Copy link

Updated by Junxiao Shi almost 10 years ago

we can remove this completely and have copy (ideally move) put.

It's sufficient to copy the Block because Impl doesn't need the Data.

Copying Block is cheap: it has less fields, and underlying Buffer is shared.

However, LpPacket needs to be constructed in the frontend Face class.

In case we want to use NDNLPv2 sequence numbers in a future feature, there would be complications.