Let me add some motivation for my API proposal:

• All the code I've seen so far that deals with body data in Rocket uses either Read::read_to_end or Read::read_to_string. Also, all of these have some kind of size limit (for obvious reasons).
• Probably most of the code wants to handle the specific error case of a too large payload, eg. for returning a 429 status code. io::Error does not seem a good error type for this case because the API consumers would need to check the error kind and try to downcast the inner error. A separate enum based error type is IMHO more convenient to use.

All in all, I'd start off with just the two functions and keep the do_with_limit private. If different use cases evolve, we can add a more comprehensive API.

@jebrosen: If you're OK with this approach, I would continue with adding documentation, some tests for the edge cases, and update all the body data consumers in Rocket and contrib.

(I'm also unsure whether take(limit + 1).read_to_end(&mut buf) might overallocate by a factor of 2 when the input data exceeds the limit)

I think it's a good idea to add this in some form, maybe with some name/API changes. Users have often wanted Vec<u8> or String in release builds; this will provide that flexibility without the same degree of risk and also make it easier to write certain FromData/FromDataSimple implementations with limit support.

In the documentation, I would definitely want to 1) encourage any potential users of these functions to use a user-configurable Limit and 2) strongly caution against using something like usize::max_value() as the limit because it's a trivial denial of service vector.

I want to revisit this, and I have some quick thoughts on the API:

• I think the methods should be on Data and take self by move
• I also think these methods should allocate a new String or Vec themselves

For example, rocket_contrib::msgpack's FromData implementation:

   let size_limit = r.limits().get("msgpack").unwrap_or(LIMIT);
   let mut buf = Vec::new();
   let mut reader = d.open().take(size_limit);
   match reader.read_to_end(&mut buf).await {
       Ok(_) => Borrowed(Success(buf)),
       Err(e) => Borrowed(Failure((Status::BadRequest, Error::InvalidDataRead(e)))),
   }

becomes

    let size_limit = r.limits().get("msgpack").unwrap_or(LIMIT);
    match d.read_to_end_with_limit(size_limit).await {
        Ok(buf) => Borrowed(Success(buf)),
        Err(e) => Borrowed(Failure((Status::BadRequest, Error::InvalidDataRead(e))))
    }

This way the methods would do what a lot of users do already, and anyone who needs to do something more complicated can still call open and read_to_end manually.

@SergioBenitez any thoughts on the above?

I think we should go even further. Here's my proposed API:

impl Data {
    fn open(limit: usize) -> DataStream;
    fn peek(&self) -> &[u8];
    fn peek_complete(&self) -> bool;
}

impl DataStream {
    fn stream_to<W: Write>(self, writer: &mut W) -> io::Result<u64>;
    fn stream_to_file<P: AsRef<Path>>(self, path: P) -> io::Result<u64>;
    fn stream_to_string(self) -> io::Result<String>;
    fn stream_to_vec(self) -> io::Result<Vec<u8>>;
}

impl Read for DataStream { /* ... */ }

I can't think of a valid reason to ever read without a limit. So, let's enforce it.

We might even consider abusing unsafe by adding an open_unsafe() method that sets no limit on the returned DataStream.

I like those methods as well. I suspect open(usize::MAX) (please never do that) is close enough to open_unsafe it's not worth a separate method.

The name stream_to_string feels a bit wrong to me compared to read_to_string -- but I do like that it gives four methods whose names all start with stream_to_.

The name stream_to_string feels a bit wrong to me compared to read_to_string -- but I do like that it gives four methods whose names all start with stream_to_.

Given that DataStream implements Read, it'll also have the read_to methods. I wanted to avoid the naming conflict.

Another thing to consider here is that limit is not a sufficient...limit...to prevent DoS attacks. While it helps mitigates memory-exhaustion based attacks, it does nothing to prevent slow-loris style attacks. On sync, this means completely tying up resources, though there we have a read-timeout, which helps but doesn't solve the problem. On async, this translates to a bunch of idling futures, which in-turn means consuming memory, which takes us right back to memory-exhaustion.

I think what we'd really like to do is expose an API that requires limits to be set on several properties, with a hard, indelible limit. In particular:

• read timeouts - how long we we're willing to wait between byte reads
• data limits - how many bytes we're willing to read in all, irrespective of time
• connection timeouts - how long we're willing to keep the connection open, irrespective of whether data is being received or not.

My guess is that a bandwidth-minimum approach to the connection timeout might actually make a bit more sense, especially when we consider very purposefully long-lived connections. That is, after a chosen period of time, assuming the other timeouts/limits haven't been exceeded, require that the bandwidth over a sliding window of time exceeds some factor. Otherwise, kill the connection, ideally in a graceful manner.

This approach seems fairly easy to implement in async. Combined with the API I proposed above, this should significantly decrease the opportunity for DoS based attacks on Rocket applications, and in the common case, make them impossible.

I'd like a comprehensive approach to this. I've opened #1325 to track such an effort. Let's move the discussion there until we have a concrete plan.