Skip to content

add addr(*mut u8) to set the desired target address #48

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

add addr(*mut u8) to set the desired target address #48

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 18, 2017

No description provided.

Copy link

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you look into the CI failure on Windows?

error[E0308]: mismatched types
  --> src\windows.rs:66:47
   |
66 |                                               addr as winapi::DWORD);
   |                                               ^^^^^^^^^^^^^^^^^^^^^ expected *-ptr, found u32
   |
   = note: expected type `*mut std::os::raw::c_void`
              found type `u32`

@danburkert
Copy link
Owner

I'm not sure that exposing MAP_FIXED is a good idea. My concern is that misuse of the API (which is easy to do) can cause existing mappings to be overwritten, including mappings created by the heap allocator.

It does seem like there is a path towards a less dangerous and cross-platform consistent API, though: we could specify the address without setting MAP_FIXED, and return an error if the address returned by mmap isn't the one requested. I've never had a usecase for MAP_FIXED, though, so I can't say if that will suffice for whatever case you have in mind.

Copy link
Owner

@danburkert danburkert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly style nits, but the MAP_FIXED issue is the bigger issue.

src/lib.rs Outdated
@@ -163,8 +166,36 @@ impl AnonymousMmapOptions {
self
}

/// Set the target address.
///
/// The address needs to be corretly aligned.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: 'correctly'

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/needs to be/must be

src/lib.rs Outdated
/// Set the target address.
///
/// The address needs to be corretly aligned.
/// If the address space is already in use, this may (on unix) replace existing mappins, or cause an error (windows).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uppercase 'Unix' and 'Windows' here and elsewhere

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: 'replace an existing mapping'

src/windows.rs Outdated
@@ -58,11 +58,12 @@ impl MmapInner {
return Err(io::Error::last_os_error());
}

let ptr = kernel32::MapViewOfFile(handle,
let ptr = kernel32::MapViewOfFileEx(handle,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reindent

src/windows.rs Outdated
@@ -94,11 +95,12 @@ impl MmapInner {
return Err(io::Error::last_os_error());
}
let access = winapi::FILE_MAP_ALL_ACCESS | winapi::FILE_MAP_EXECUTE;
let ptr = kernel32::MapViewOfFile(handle,
let ptr = kernel32::MapViewOfFileEx(handle,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reindent

src/lib.rs Outdated

/// Set the target address hint.
///
/// Insuficcient alignment will cause the mapping to fail on windows.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be more clear as 'The address must be aligned to the system pagesize.'

@ghost
Copy link
Author

ghost commented Aug 1, 2017

@danburkert My usecase does require MAP_FIXED. (I need a bigger alignment that the page-size.)
Without MAP_FIXED the returned address is a bit like gambling on Linux. sometimes you get what you requested in the first attempt, sometimes you don't even get it after 10 attempts.

@dtolnay dtolnay mentioned this pull request Aug 6, 2017
@danburkert
Copy link
Owner

@s3bk not sure I follow. If you need a bigger alignment than page size, you should over-allocate and then start using the map from the closest offset matching your desired alignment.

I've never used MAP_FIXED, but my interpretation from the docs is that if you used MAP_FIXED to work around the memory address you are requesting already being mapped, there's no way to tell the previous user of that memory space that it's been preempted. Needless to say, this would be very unsafe.

@danburkert
Copy link
Owner

Note about 1.0: regardless of how we land on whether MAP_FIXED should be exposed, I do think we should allow setting the address, however I'm confident this can be done backwards compatibly with the new builder pattern post 1.0, so I don't consider this feature a blocker.

@danburkert danburkert mentioned this pull request Aug 17, 2017
@ghost
Copy link
Author

ghost commented Aug 17, 2017

@danburkert

If you need a bigger alignment than page size, you should over-allocate and then start using the map from the closest offset matching your desired alignment.

That would work, if it wasn't using a file (I don't think negative offsets are allowed)

Creating a dummy mmap to pad the area up to the desired alignment would probably work.

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 this pull request may close these issues.

2 participants