Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert mmap()s to smart pointers #32532

Closed
gabrielschulhof opened this issue Mar 28, 2020 · 6 comments
Closed

Convert mmap()s to smart pointers #32532

gabrielschulhof opened this issue Mar 28, 2020 · 6 comments
Assignees

Comments

@gabrielschulhof
Copy link
Contributor

Re: #32396 (comment)

@gabrielschulhof
Copy link
Contributor Author

@jasnell said

Alternatively, it may be a bit cleaner to use a custom smart pointer with a deleter for these...

e.g. bit more verbose...

  struct Mmap {
    void* mem = nullptr;
    size_t size = 0;
    Mmap(void* start, size_t size, int prot, int flags) {
      mem = mmap(start, size, prot, flags, -1, 0);
    }
  };
  struct MmapDeleter {
    void operator()(Mmap* ptr) const noexcept {
      if (ptr.mem != nullptr &&
          ptr.mem != MAP_FAILED &&
          munmap(ptr.mem, ptr.size) == -1)
        PrintSystemError(errno); 
    }
  };
  using MmapPointer = std::unique_ptr<Mmap, MmapDeleter>;

then...

  MmapPointer nmem;
  MmapPointer tmem;

  // ...
  size_t size = r.to - r.from; 

  // ...
  nmem = std::make_unique(nullptr, size,
                          PROT_READ | PROT_WRITE,
                          MAP_PRIVATE | MAP_ANONYMOUS);
  if (nmem.mem == MAP_FAILED)
    goto fail;

  // ...
  tmem = std::make_unique(start, size,
                          PROT_READ | PROT_WRITE,
                          MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED);
  if (tmem.mem == MAP_FAILED)
    goto fail;

@gabrielschulhof
Copy link
Contributor Author

@jasnell @addaleax IINM it would introduce an additional level of indirection to go with std::unique_ptr, because the structure Mmap would have to be heap-allocated and then the void* would be stored within that.

@gabrielschulhof
Copy link
Contributor Author

AFAICT std::unique_ptr is good for pointer types that can be freed with FreeThisPointerType(PointerType* ptr); If an additional argument needs to be given to the deleter then that argument, along with the pointer, must be stored in a structure, which becomes heap-allocated if we use std::unique_ptr.

@addaleax
Copy link
Member

@gabrielschulhof Right, I think this is sometime the Mmap (or let’s be verbose and call it MemoryMapping) class can take of itself in its constructor/destructor. std::unique_ptr or friends don’t really seem to make sense here to me either.

@gabrielschulhof
Copy link
Contributor Author

That's what I was thinking too. A simple class which, in its constructor performs mmap(2) and its destructor performs munmap(2).

@jasnell
Copy link
Member

jasnell commented Mar 30, 2020

That works for me

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 3, 2020
Replace `OnScopeLeave` with a class whose instance destructor performs
the munmap(2).

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: nodejs#32532
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
BethGriggs pushed a commit that referenced this issue Apr 7, 2020
Replace `OnScopeLeave` with a class whose instance destructor performs
the munmap(2).

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: #32532
PR-URL: #32570
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this issue Apr 12, 2020
Replace `OnScopeLeave` with a class whose instance destructor performs
the munmap(2).

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: #32532
PR-URL: #32570
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit to targos/node that referenced this issue Apr 25, 2020
Replace `OnScopeLeave` with a class whose instance destructor performs
the munmap(2).

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: nodejs#32532
PR-URL: nodejs#32570
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this issue Apr 28, 2020
Replace `OnScopeLeave` with a class whose instance destructor performs
the munmap(2).

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Fixes: #32532
PR-URL: #32570
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants