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

[strict provenance] Provide a way to "create allocations at a fixed address" #98593

Open
RalfJung opened this issue Jun 27, 2022 · 83 comments
Open
Labels
A-strict-provenance Area: Strict provenance for raw pointers T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. WG-embedded Working group: Embedded systems

Comments

@RalfJung
Copy link
Member

RalfJung commented Jun 27, 2022

This issue is part of the Strict Provenance Experiment - #95228

On some platforms it makes sense to just take a hard-coded address, cast it to a pointer, and starting working with that pointer -- because there are external environment assumptions that say that certain things are provided at certain addresses.

This is perfectly fine for Rust as long as that memory is entirely disjoint from all the memory that Rust understands (static globals, stack allocations, heap allocations). Basically we can think of there being a single hard-coded provenance for "all the memory that is disjoint from the Abstract Machine", and that is the provenance we would like to use for these pointers created from hard-coded addresses. These restrictions make this operation a lot easier to specify than from_exposed_addr. (Remember: from_exposed_addr is outside of Strict Provenance. The goal of this issue is to provide a way to write such code while following Strict Provenance.)

In the spirit of the Strict Provenance APIs, that means we probably want a function that does this, and that we can attach suitable documentation to. There are some open questions for the syntax and semantics of that function:

  • What should it be called? make_alloc, assume_alloc, hard_coded_alloc? I am leaning towards something with "alloc" because this function is basically like an allocator, except that you tell it at which address to allocate and you have to promise that that is Okay To Do.
  • It should definitely take a usize for the address and return *mut T. Should it also take a size, saying how large this assumed allocation is?
  • Should each invocation return a fresh provenance, or should they all return the same provenance? Probably the latter; using a fresh provenance comes with a bunch of restrictions that I doubt such low-level code will be happy about.
  • (other things I have not thought of)

Cc @Lokathor who keeps mentioning this usecase every time I want to ban int2ptr casts. ;)
Tagging WG-embedded since that's where this kind of stuff mostly happens (AFAIK)

@RalfJung RalfJung added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. WG-embedded Working group: Embedded systems A-strict-provenance Area: Strict provenance for raw pointers labels Jun 27, 2022
@RalfJung
Copy link
Member Author

Until such a function exists, one can use from_exposed_addr_mut for this purpose. The "provenance for all things outside the Abstract Machine" is definitely exposed and can hence be picked up by that function.

@Lokathor
Copy link
Contributor

One thing I'd like to mention at the start is that hardware addresses should not be modeled as "owned" by anything inside Rust. They're generally volatile, and generally what you read from them might change without Rust doing anything on its end. At best we should think of them as Rust sharing the address with some external force.

Because of this, I think a name like "alloc" would be a misstep. People have the notion/understanding that when you alloc some memory you own it while it's live, and then you (or some drop glue) explicitly does a "free", and and it's dead. That's not really how hardware access works at all, so for a new situation we pick a new name.

Particularly, lots of existing and well working rust code doesn't bother with a "free" step at all when using hardware pointers. You just make up a pointer that you know is good, you read or write, and then you forget the pointer, and someone else might even be holding a pointer to that same location while you're doing all this, and it's all fine. If a person wants to build an ownership-style API on top of this using the borrow checker they can do that (there's several such examples), but that's separate from the base requirements of the abstract machine interacting with the hardware.

@clarfonthey
Copy link
Contributor

Perhaps directly labelling these addresses as volatile might be the best way to go? This would also help for FFI as well, since you could make the distinction between an address for something that is "owned" inside the Rust code until passed back via FFI, or something that could concurrently be modified outside of Rust, by hardware or another thread.

@RalfJung
Copy link
Member Author

@Lokathor what you say also confirms my thoughts above that we want to use the same provenance for all calls to this function, and not generate a fresh one. That provenance is assumed to be exposed from the start, so it is basically public and can change any time control is given to other code (including via another thread).

Using volatile accesses is a separate concern; Rust assumes for all pointers that if you do 2 non-atomic loads immediately after one another, they will give the same result -- there is no good way to opt-out of that assumption just for a specific provenance; this needs a more explicit marker at the relevant access. (I.e., making it volatile.)

Perhaps directly labelling these addresses as volatile might be the best way to go?

There is no such thing as a volatile address. Being volatile is the property of an access.
If you want to ensure that all accesses to an address are volatile, that's fine, but this can't be automatic or else Rust would have to assume that any raw pointer access might have to be volatile.

There are plenty of discussions around the semantics of volatile, their interaction with reference types, and so on; I would like to keep them out of here. This thread is solely about the name, signature, and provenance interaction of the function that is intended to replace those casts.

@Lokathor

This comment was marked as off-topic.

@RalfJung

This comment was marked as off-topic.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 27, 2022

There are plenty of cases where you want access at a particular address that has nothing to do with volatile. For example, you might write a kernel and know that the firmware put some data structure with important info at a hard-coded address.

So, this API certainly should not be tied to volatile in any way. Just like ownership/borrowing, APIs for convenient (and maybe even safe) volatile access can be built on top of this lower-level primitive. That's why I marked the previous posts about volatile as off-topic.

@adamgreig
Copy link
Member

I'll raise this at the next embedded WG meeting (tues 28th 20:00 CEST) to see if anyone else has thoughts.

It should definitely take a usize for the address and return *mut T. Should it also take a size, saying how large this assumed allocation is?

I'd think so, we will often have a pointer to a large struct or array or quite possibly an entire memory region that is only for MMIO with fixed addresses (e.g. on Cortex-M, 0x4000_0000 through 0x5FFF_FFFF is all peripheral MMIO).

@RalfJung
Copy link
Member Author

I'd think so, we will often have a pointer to a large struct or array or quite possibly an entire memory region that is only for MMIO with fixed addresses (e.g. on Cortex-M, 0x4000_0000 through 0x5FFF_FFFF is all peripheral MMIO).

And then would you want it to be UB to use that pointer outside the given range? Or what would the exact consequence be of setting a particular size?

@adamgreig
Copy link
Member

If it didn't take a size, would you assume a single word provenance or the whole memory, like an escaped pointer? I was imagining you'd want to be able to restrict it to the smallest usable provenance, which is generally known statically or easily bounded to a memory region.

If there's not really any advantage to the compiler to knowing the possible valid range of the pointer, then I suppose just taking the address and giving it a whole-memory provenance is a simpler API.

My guess for a typical embedded use-case is you'd create one of these pointers for each instance of a peripheral on your chip, so each has a non-overlapping size of say <100 words, and probably create them on-demand each time you accessed the peripheral.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 27, 2022

If it didn't take a size, would you assume a single word provenance or the whole memory, like an escaped pointer?

It would be allowed to access all memory that is outside of what the Abstract Machine knows about (static, stack, heap, anything internal to the implementation like compiler-generated data).

probably create them on-demand each time you accessed the peripheral.

This means multiple calls to that function can overlap anyway, so a size does not give any disjointness guarantees.

@Lokathor
Copy link
Contributor

A size/span restriction does not seem useful.

@adamgreig
Copy link
Member

It would be allowed to access all memory that is outside of what the Abstract Machine knows about (static, stack, heap, anything internal to the implementation like compiler-generated data).

Right, I should have read the issue more carefully. I agree then, it doesn't seem like there'd be any benefit to giving a size, even if it is known.

For naming, I'm also not entirely sure about saying alloc; maybe the thing to express is that we're making a pointer that shares some global disjoint-from-known-universe provenance with all other similarly-created pointers? I think extern has too many other meanings, but if it didn't, something like make_extern or assume_extern could work.

@jamesmunns
Copy link
Member

Thinking a bit about the use cases here, just to make sure I understand:

  • Fixed memory map addresses (e.g. MMIO pointers)
  • Runtime mapped regions (virtual memory mapping?), created by the code itself
  • Runtime mapped regions done indirectly from the code (i.e. mmap - this might have a separate function for provenance? Or is this just handled by "pointer escaping" in the compiler currently?)

You mention specifically "for these pointers created from hard-coded addresses", would the latter two runtime-y cases also follow/benefit from this API?

If all three are relevant, I'd propose make_mapped or assume_mapped (or maybe make|assume_extern_mapped, as these all seem to derive from cases where we have memory that has been mapped (either statically or at runtime) into an address space outside of Rust's control.

@Lokathor
Copy link
Contributor

Lokathor commented Jun 28, 2022

"external address" seems like a good term, <*mut T>::from_extern(usize) seems maybe like a fine way to make them.

@RalfJung
Copy link
Member Author

Yeah I considered "extern", too -- but the term is awfully overloaded.

You mention specifically "for these pointers created from hard-coded addresses", would the latter two runtime-y cases also follow/benefit from this API?

This is specifically to replace current uses of int as *mut PTR. Functions like mmap already return a pointer and that one for all intents and purposes will have a fresh provenance -- they are like malloc, I think.

@jamesmunns
Copy link
Member

That makes sense for the third point!

My second item is more for implementing an operating system, where you might want to map a page, zero it, then pass it on to "userspace".

If you were implementing ASLR for example, you'd (more or less) be generating the base mapped address from an RNG, which would probably have come from some kind of integer rather than pointer. That being said, the OS itself probably sees the physical memory address the same as case one. Once the OS establishes the pointer, and passes it to userspace (where userspace is in case three), you're right that userspace doesn't have to re-establish provenance, I guess.

@Dirbaio
Copy link
Contributor

Dirbaio commented Jun 28, 2022

"foreign" maybe? so there's "program memory" and "foreign memory". The function could be ptr::from_foreign_addr().

@RalfJung
Copy link
Member Author

@jamesmunns from the kernel perspective this is memory outside of what Rust knows about, so if there are int2ptr casts there then using this function probably makes sense.

However now that if you e.g. implement a global allocator, then the memory return by that is considered to be "known to Rust" and thus must not be accessed with that "external"/"foreign" provenance.

@A1-Triard
Copy link

A1-Triard commented Jun 28, 2022

Functions like mmap already return a pointer and that one for all intents and purposes will have a fresh provenance -- they are like malloc, I think.

But what if I want to perform mmap syscall using asm? Looks like you need a way to create a new, unique provenance for a such case.

@Lokathor
Copy link
Contributor

Lokathor commented Jun 28, 2022

inline asm provenance creation rules should be the same as FFI provenance creation rules (whatever those are).

I don't know if that's fully decided yet, but in all other situations inline asm works "basically like FFI with a weird calling convention", and I see no reason to break from that convention.

@RalfJung
Copy link
Member Author

asm! block semantics are axiomatized anyway. You just have to be able to describe what happens in terms of the Abstract Machine state (in a way that is consistent with the actions of the asm! on the physical state); so that description can just state that it generates a fresh provenance since that is something the Abstract Machine can do.

@programmerjake
Copy link
Member

one other case to consider is where the program picks an address, and then memmaps to that address, using something like MAP_FIXED:

let addr: *mut u8 = 0x12340000 as *mut u8; // pick an arbitrary page-aligned address
let addr = mmap(addr as *mut c_void, 0x10000, PROT_READ | PROT_WRITE, MAP_FIXED | MAP_PRIVATE, fd, 0);

imho mmap here should return the same provenance as the addr argument even though that addr argument wasn't valid until the mmap call.

@RalfJung
Copy link
Member Author

imho mmap here should return the same provenance as the addr argument even though that addr argument wasn't valid until the mmap call.

That sounds pretty reasonable to me.
In particular you also must stay clear of any Rust-managed memory when doing this.

@algesten
Copy link
Contributor

In the embedded code I've encountered (STM32 mainly), this is the common pattern.

unsafe {
    // NOTE(unsafe) this reference will only be used for atomic writes with no side effects.
    let rcc = &(*RCC::ptr());

    // Enable clock.
    $GPIOX::enable(rcc);
    $GPIOX::reset(rcc);
}

There will be an underlying crate that is mostly generated from the MCU vendor's SVD-files (often with some patch set with correction on top). The maps each MCU peripheral to a specific memory address RCC::ptr() etc.

impl GPIOA {
    pub const PTR: *const gpioa::RegisterBlock = 0x4002_0000 as *const _;
    pub const fn ptr() -> *const gpioa::RegisterBlock {
        Self::PTR
    }
}

The pointer will be into some block, probably "Peripherals" below starting 0x4000_0000 (from reference manual).

image

For the proposed provenance API, are we saying that we will declare the entire Peripheral block on startup and then take additional offsets from inside that block for the individual pointers?

Given the current embedded landscape, I believe that having an API to declare provenance for an individual pointer would be less of an upheaval than going via a block.

(As an aside, I'm also not keen on the name alloc for this – not sure what I'd call it though)

@RalfJung
Copy link
Member Author

Would it make sense to make it "exposed by default"? This would mean existing embedded code that does stuff like 0x4000_0000 as *const RegisterBlock would keep being correct in Strict Provenance (assuming int2ptr as expands to from_exposed_addr).

Yes that was my intent. We need to keep that code working for backwards-compatibility anyway.
Any "globally well-known provenance" should be considered exposed. The reason we need to distinguish "exposed" vs "non-exposed" is that when some code generates a fresh provenance in the privacy of its own function (e.g. for their stack variables, or via malloc), we need to make sure nobody else can just get hold of that provenance via from_exposed_addr.

@Dirbaio
Copy link
Contributor

Dirbaio commented Jun 29, 2022

Makes sense.

Why do we need a from_foreign_addr if from_exposed_addr already works, then?

It could enable better optimizations, since the compiler knows from_foreign_addr will never touch exposed program memory. Would these make a difference in practice though? Most code out there will conform to Strict Provenance, so most program memory won't be exposed

@RalfJung
Copy link
Member Author

It was mostly meant as an explicit bit of documentation, i.e., the programmer making their intent more clear.

@digama0
Copy link
Contributor

digama0 commented Jun 29, 2022

Especially if it is "the programmer making their intent more clear" I think there is use for the size field. Since as far as AM operations are concerned, the intrinsic is already superfluous since from_exposed_addr also works. Unless you want to say that from_foreign_addr on an exposed Rust allocation is UB?

@RalfJung
Copy link
Member Author

RalfJung commented Jun 29, 2022

By that standard, why do we have any of the other functions (ptr::invalid, with_addr)?

The idea is to provide ways to do all these things while following Strict Provenance. And that means not using from_exposed_addr. If you are okay with using from_exposed_addr, you don't need to worry about anything re: Strict Provenance, you just decided to use a more permissive model of provenance.

Unless you want to say that from_foreign_addr on an exposed Rust allocation is UB?

Yes, I want to say that. You cannot access a Rust allocation, exposed or not, with the "foreign" provenance.

@A1-Triard
Copy link

@RalfJung , can you imagine a situation when difference between from_exposed_addr, and from_foreign_addr would mean something in practice? As longer I think about it, as more artificial from_foreign_addr seems to me. Maybe it is better to accept that such situation breaks strict provenance unavoidably.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 2, 2022

It's not unavoidable though. 🤷

from_exposed_addr is a non-deterministic operation, i.e. executing it twice on the same input could produce two different results. So that could make a difference for the optimizer, when compared with from_foreign_addr which is deterministic.

@DemiMarie
Copy link
Contributor

I agree in principle UB is dangerous, but you are not exactly specifying an alternative defined behaviour. Rather, nothing is being specified at all with regards to the extent to which such a pointer can be used. Are you not already imagining making it UB to use such a pointer to access memory that is a part of the memory rust understands for its heap and stack?

Yes. That is the amount of UB we need. Beyond that, it is also UB if the load causes a fault or data race. But everything else we can should not make UB.

Forbidding data races is okay. Forbidding faults is in general not okay, as it would prevent legitimate uses of faults such as pagable kernel heap memory. That actually has an additional constraint, which is that it can be safe to access some memory memory at time A or at time C, but not at time B (between A and C) because time B is one at which paging is not permitted.

In short, for kernels and similarly low-level code, one wants to be able to reorder ordinary non-volatile loads and stores across most operations, but not across arbitrary function calls, because these might make the memory temporarily inaccessible.

@DemiMarie
Copy link
Contributor

Another point to keep in mind: Accessing freed memory is UB. However, the memory allocator (almost by definition) needs to do exactly that!

@RalfJung
Copy link
Member Author

RalfJung commented Jul 3, 2022

Forbidding faults is in general not okay,

Sorry, I should have been more precise -- by "fault" I meant something that the program can actually observe. If the kernel or some other entity does some magic in the background, that is fine. But, crucially, all memory accesses must succeed and not have any side-effect besides reading/writing the given memory location.

Otherwise, the compiler would not even be allowed to swap the order of two adjacent reads.

In short, for kernels and similarly low-level code, one wants to be able to reorder ordinary non-volatile loads and stores across most operations, but not across arbitrary function calls, because these might make the memory temporarily inaccessible.

They will have to add suitable barriers or other annotations then. Rust is allowed to reorder some reads around arbitrary function calls. (In fact even C is allowed to do that, if the read-from memory is private to the current function. Rust is just allowed to do it in more situations due to its aliasing restrictions.)

Anyway this is off-topic for this thread, since we are not talking about memory accesses here, only about how to get the pointer to some particular kind of memory. This has zero impact on the optimizations that Rust can do when it does not know where the pointer comes from, so everything that the compiler is allowed to do for arbitrary references / raw pointers, it is also allowed to do for references / raw pointers derived from from_foreign_addr.

@scottmcm
Copy link
Member

scottmcm commented Jul 3, 2022

Particularly, lots of existing and well working rust code doesn't bother with a "free" step at all when using hardware pointers.

I don't know that that means "alloc" is an inappropriate word, though. We describe static as an "allocation" that's definitely never freed either.

@repnop
Copy link
Contributor

repnop commented Jul 12, 2022

on the name bikeshedding, what about something like reveal_alloc? since you're revealing the memory "allocation" to the AM.

I'm ambivalent on taking a size or not. on one hand, generally its known in some form when you're working with MMIO (there's usually a block of the memory space allocated to the device even if some or most of it is unusable), but what size should you use in the case of not knowing? (as unlikely as that seems to me) the point is that this memory is outside of the AM and thus isn't allowed to overlap it, right? would using {i, u}size::MAX (or a sufficiently large N) as an argument to it be UB since it then would almost definitely overlap with AM memory?

@lukas-code
Copy link
Contributor

@digama0
Copy link
Contributor

digama0 commented Aug 8, 2022

I think this will always be UB. Creating the allocation is probably fine, but actually using *ptr where ptr is equal to 0 should be UB. If you want to dereference 0, you should use asm!.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 8, 2022

Currently, dereferencing a null pointer seems to be always UB, even if it was obtained with from_exposed_addr(0).

Indeed, Rust assumes that there is never an allocation at 0, so there can't be an exposed provenance that is valid for that address, either.

@DemiMarie
Copy link
Contributor

I think this will always be UB. Creating the allocation is probably fine, but actually using *ptr where ptr is equal to 0 should be UB. If you want to dereference 0, you should use asm!.

Only non-volatile access should be UB. If my proposal for volatile semantics is accepted, one can use volatile to access any address whatsoever, including address 0 and even freed memory. The reason this does not harm optimizations is that the return value of ptr::read_volatile in these situations is unspecified (not undefined — it won’t be undef or poison). Having to use asm for this would be silly.

@RalfJung
Copy link
Member Author

The general conclusion in this thread regarding the original question seems to be: why have a ptr::extern or so if one can use ptr::from_exposed_addr instead? Here is one argument: stabilizing from_exposed_addr is further off than stabilizing the other functions, because of their rather unconventional semantics (angelic non-determinism) that should probably be studied a bit more before it gets enshrined in stable Rust. We are hence probably going to stabilize the core strict provenance functions (addr, with_addr, map_addr, invalid) before the once involving 'expose'.

Therefore, having a dedicated function for this case of accessing an 'external' address could help users of stable Rust away from as casts earlier. It could also help document intent, clearly saying that this is not relying on any address having been previously exposed, but instead relies on the address being 'external' to the Rust AM.

@Lokathor
Copy link
Contributor

Lokathor commented Nov 21, 2022

Currently, when using as, the user doesn't have to worry about if the usize is previously a runtime exposed pointer or a hardware address, they just do the as cast. This has less chance of screwing up, because it's one less thing that matters. So personally I wouldn't want to use extern_addr or anything like that, I'd just wait longer for from_exposed_addr to stabilize.

EDIT: as one example: it would break all my tests in voladdress if I can't allocate a vec and work within the vec the same as I can use real hardware access.

@LunarLambda
Copy link

Personally I think "each calls returns a unique provenance of a given size" makes the most sense. It covers the most use cases with the least API surface area. Whole memory regions, splitting memory regions, "magical" allocators, etc.

I'm not sure how useful it is to have "The One Unknowable Everything-Else Provenance" (i.e. no size parameter, single provenance for all "external"/non-rust addresses). Feels like that's only a very slightly smaller hammer than from_exposed_addr, and the only use it serves is "being able to arbitrarily offset into all possible external memory", which I think is basically never done outside of asm, which should already be covered by the FFI-ish provenance?

On that note, what about memory regions that can be relocated at runtime? Is this something that can be at all expressed under strict provenance (other than for discarding and re-creating the existing provenances)? I thought of with_addr, but the documentation says it's equivalent to wrapping_offset, which in turn says the pointer must not leave the memory range of the allocation, which I think implicitly assumes that the allocation cannot move?

@RalfJung
Copy link
Member Author

RalfJung commented Nov 7, 2023

Allocated objects in Rust cannot move. So such relocations (in general) have to be modeled like realloc, and they generate a fresh provenance, where all old pointers now have a dangling provenance that must not be used any more.

@joboet
Copy link
Contributor

joboet commented Dec 19, 2023

Supposing that this function did take a size, what would the semantics of a pointer created with size zero be? Would calling offset(n) with $n \ne 0$ on it be undefined behaviour, or would it behave the same way as for a pointer returned by ptr::invalid (or whatever that function will be named in the future)?

@LunarLambda
Copy link

Doesn't Rust disallow memory allocations of size 0 (which is why Vec/Box/etc have special cases for ZSTs)?

I would assume the same to apply here

@joboet
Copy link
Contributor

joboet commented Dec 19, 2023

It does not.

@LunarLambda
Copy link

https://doc.rust-lang.org/nightly/std/alloc/trait.GlobalAlloc.html#safety-1 says it's UB to request allocations of size 0, which is different from creating an invalid pointer which is okay to be used for accesses of size 0

So my assumption is "Rust allocations cannot be zero-sized" would hold here?

@RalfJung
Copy link
Member Author

RalfJung commented Dec 19, 2023

Heap allocations of size zero cannot be created with our global allocator. However, stack variables and global static with size 0 can exist and those are allocations, too.

@programmerjake
Copy link
Member

heap allocations of size zero can be created with some implementations of malloc, e.g.glibc 2.38:
https://sourceware.org/git/?p=glibc.git;a=blob;f=malloc/malloc.c;h=e2f1a615a4fc7b036e188a28de9cfb132b2351df;hb=36f2487f13e3540be9ee0fb51876b1da72176d3f#l115

@Lokathor
Copy link
Contributor

If only some native allocators support it then rust overall must assume it's not supported.

@programmerjake
Copy link
Member

If only some native allocators support it then rust overall must assume it's not supported.

yes, except that Rust can't declare that no zero-sized heap allocations are possible, they just can't be done with Rust's standard methods of allocating memory.

@joboet
Copy link
Contributor

joboet commented Dec 19, 2023

Supposing that this function did take a size, what would the semantics of a pointer created with size zero be? Would calling offset(n) with n≠0 on it be undefined behaviour, or would it behave the same way as for a pointer returned by ptr::invalid (or whatever that function will be named in the future)?

I'm asking, because if so, it would make sense to implement ptr::invalid via this new function and avoid another intrinsic once strict-provenance is implemented using those.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 20, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-strict-provenance Area: Strict provenance for raw pointers T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. WG-embedded Working group: Embedded systems
Projects
None yet
Development

No branches or pull requests