Project

General

Profile

Actions

Bug #3678

closed

Face::put(const Data&) undefined behavior

Added by Junxiao Shi over 7 years ago. Updated over 7 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Base
Target version:
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 #1

Updated by Junxiao Shi over 7 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 #2

Updated by Davide Pesavento over 7 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 #3

Updated by Junxiao Shi over 7 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 #4

Updated by Alex Afanasyev over 7 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 #5

Updated by Junxiao Shi over 7 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.

We may also rename put to "putData" for consistency with NDN-CCL.

No. Consistency with NDN-CCL is a non-goal.

Actions #6

Updated by Junxiao Shi over 7 years ago

  • Status changed from New to In Progress
  • Assignee set to Junxiao Shi
  • Target version set to v0.5
  • Estimated time set to 3.00 h
Actions #7

Updated by Junxiao Shi over 7 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 0 to 100

https://gerrit.named-data.net/3052 implements note-5 design.

Actions #8

Updated by Junxiao Shi over 7 years ago

  • Status changed from Code review to Closed
Actions

Also available in: Atom PDF