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 over 8 years ago. Updated over 8 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 over 8 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 over 8 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 over 8 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 over 8 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 over 8 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.