2

According to cppreference calling std::unique_ptr::operator*() is equivalent to calling *(std::unique_ptr::get()).

However I'm getting different results for both calls. Here is my code:

#include <iostream>
#include <string>
#include <memory>

#include <fcntl.h>
#include <unistd.h>

struct file_descriptor
{
private:
  struct closer;

public:
  typedef int handle_type;
  typedef closer closer_type;

  constexpr static handle_type kInvalidHandle = -1;

public:
  file_descriptor(int handle = kInvalidHandle) : handle_{ handle } { }
  file_descriptor(std::nullptr_t) : file_descriptor{ } { }

  operator int&() { return handle_; }
  operator int() const { return handle_; }

  int& operator*() { return static_cast<int&>(*this); }
  int operator*() const { return static_cast<int>(*this); }

  bool operator==(const file_descriptor& other) const
  { return (handle_ == other.handle_); }

  bool operator!=(const file_descriptor& other) const
  { return !(*this == other); }

private:
  struct closer
  {
    typedef file_descriptor pointer;

    void operator()(pointer handle) const
    { ::close(*handle); }
  };

  int handle_;
};

using unique_file_ptr = std::unique_ptr<typename file_descriptor::handle_type,
                                        typename file_descriptor::closer_type>;

unique_file_ptr managed_open(const std::string& path)
{
  return { ::open(path.c_str(), O_RDWR), { } };
}

int main(int, char**)
{
  auto handle = managed_open("/dev/random");
  std::cout << "*handle      : " << *handle << std::endl;
  std::cout << "*handle.get(): " << *handle.get() << std::endl;
}

My output (live output here):

*handle      : 4198400
*handle.get(): 3

Please note that *handle.get() returns the correct value, while *handle doesn't.

Why am I getting different results?

9
  • @Lingxi No: decltype(&::open) = int (*)(const char *, int, ...). I guess it's because file_descriptor is constructible from an int.
    – Tom Knapen
    Apr 3, 2015 at 15:54
  • 1
    @RichardHodges Funnily enough, when I comment out the operator*()'s, the following error occurs: .../include/c++/4.8/bits/unique_ptr.h:222:9: error: indirection requires pointer operand ('pointer' (aka 'file_descriptor') invalid) return *get();, which means that unique_ptr::operator*() effectively is equivalent to *get(). This leads me to believe I'm somehow invoking UB in my code.
    – Tom Knapen
    Apr 3, 2015 at 16:08
  • 1
    The return static_cast<int&>(*this); line looks really smelly. What are you trying to do here? Apr 3, 2015 at 16:15
  • 1
    unique_file_ptr::element_type is file_descriptor::handle_type, while unique_file_ptr::pointer is file_descriptor. I'm just double-bent ~>_<~
    – Lingxi
    Apr 3, 2015 at 16:15
  • 1
    @RichardHodges The result from open will automatically be converted to a pointer (aka file_descriptor) before being passed to unique_ptr's constructor (because unique_ptr's constructor takes a pointer by value). The unique_ptr doesn't store an int, it stores a file_descriptor.
    – Tom Knapen
    Apr 3, 2015 at 16:32

2 Answers 2

6

Here's what happens. unique_ptr<T, D>::get() returns D::pointer - in your case, a temporary file_descriptor constructed from the int handle. Then its operator* calls its operator int& which returns a reference to handle_ stored inside that temporary.

When you call *handle.get() directly, the int& reference it produces is used before the temporary dies.

But when you call *handle instead, you call handle.operator*() which in turn calls handle.get(). The implementation of operator*, with everything expanded, becomes something like this:

int& operator*() {
  file_descriptor temp(internal_pointer_);
  int& result = temp.handle_;
  return result;
}

The function returns a reference to the member of a temporary. That temporary dies at the closing brace, and the reference becomes dangling, whereupon the program exhibits undefined behavior triggered by accessing an object after its lifetime has ended.

6
  • 1
    So unique_ptr::pointer should better be a real pointer type :p
    – Lingxi
    Apr 3, 2015 at 16:43
  • I came to the same conclusion while sifting through the code for unique_ptr in my stdlib. And while some may argue the usability of my code, I was just playing around with unique_ptr since it seemed like an easy way to get some RAI behaviour. I'm just gonna try to find a way to make this work with unique_ptr (for educational purposes).
    – Tom Knapen
    Apr 3, 2015 at 16:53
  • 1
    @Lingxi Not necessarily. It's OK for it to be a proxy class - but the proxy object shouldn't be making a copy of the object being proxied. Having said that, in this particular example file_descriptor doesn't appear to serve any useful purpose whatsoever. Apr 3, 2015 at 16:55
  • @IgorTandetnik Indeed. In the questionable code, unique_ptr::pointer is the real entity type file_descriptor with no indirection.
    – Lingxi
    Apr 3, 2015 at 16:58
  • @TomKnapen I think the obscure relationship between unique_file_ptr::element_type and unique_file_ptr::pointer is what breaks the original code.
    – Lingxi
    Apr 3, 2015 at 17:06
0

I suggest you re-think the solution. Here's an arguably cleaner way to approach it:

#include <iostream>
#include <string>
#include <memory>
#include <stdexcept>
#include <utility>

#include <fcntl.h>
#include <unistd.h>

struct managed_file {
    managed_file() noexcept {};
    managed_file(const std::string& path) 
    : _handle { ::open(path.c_str(), O_RDWR) }
    {
        if (_handle == -1) {
            throw std::runtime_error { std::string { "failed to open file " } + path };
        }
    }

    managed_file(const managed_file&) = delete;
    managed_file(managed_file&& other) noexcept 
    : _handle { other._handle }
    {
        other._handle = -1;
    }

    managed_file& operator=(const managed_file&) = delete;
    managed_file& operator=(managed_file&& other) noexcept {
        managed_file tmp { std::move(other) };
        using std::swap;
        swap(_handle, other._handle);
        return *this;
    }

    virtual ~managed_file() noexcept 
    {
        close();
    }

    void close() noexcept {
        if (_handle != -1) {
            ::close(_handle);
            _handle = -1;
        }
    }

    const int& operator*() const noexcept {
        return _handle;
    }

private:
    int _handle = -1;
};

managed_file managed_open(const std::string& path)
{
    return managed_file { path };
}

using namespace std;

int main(int, char**)
{

    cout << "opening" << endl;
    auto handle = managed_open("/dev/random");

    cout << "checking" << endl;
    std::cout << "*handle      : " << *handle << std::endl;
}
2
  • I guess the OP is trying to reuse std::unique_ptr.
    – Lingxi
    Apr 3, 2015 at 16:54
  • Of course I understand that - but it's not a natural fit the way he's trying to do it. Apr 3, 2015 at 17:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.