Daniel Shahaf wrote:
> But... isn't there a bug here?
>
> [[[
> Index: subversion/libsvn_ra_svn/marshal.c
> ===================================================================
> --- subversion/libsvn_ra_svn/marshal.c (revision 1133231)
> +++ subversion/libsvn_ra_svn/marshal.c (working copy)
> @@ -285,14 +285,15 @@ static svn_error_t *readbuf_input(svn_r
>
> apr_size_t *len, apr_pool_t *pool)
> {
> svn_ra_svn__session_baton_t *session = conn->session;
> + apr_size_t wanted = len;
>
> if (session && session->callbacks &&
> session->callbacks->cancel_func)
> SVN_ERR((session->callbacks->cancel_func)(
> session->callbacks_baton));
>
> SVN_ERR(svn_ra_svn__stream_read(conn->stream, data, len));
> - if (*len == 0)
> + if (*len < wanted)
> return svn_error_create(SVN_ERR_RA_SVN_CONNECTION_CLOSED, NULL, NULL);
>
> if (session)
> ]]]
>
> >From svn_stream_t doxygen:
>
> * Handlers are obliged to complete a read or write to the maximum
> * extent possible; thus, a short read with no associated error implies
> * the end of the input stream, and a short write should never occur
> * without an associated error.
I'm not sure what you're claiming. If you're suggesting we should apply
the tweak above, that would make a "short read" throw a "hard" error (by
which I mean one that's not handled as EOF), whereas the doc string you
quoted says a short read should be treated as EOF.
That said, I can't quite follow why the code you quoted throws an error
if it reads no data.
> > -/* Read LEN bytes from CONN into a supposedly empty STRINGBUF.
> > - * POOL will be used for temporary allocations. */
> > -static svn_error_t *
> > -read_long_string(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> > - svn_stringbuf_t *stringbuf, apr_uint64_t len)
> > +/* Read LEN bytes from CONN into already-allocated structure ITEM.
> > + * Afterwards, *ITEM is of type 'SVN_RA_SVN_STRING', and its string
> > + * data is allocated in POOL. */
> > +static svn_error_t *read_string(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> > + svn_ra_svn_item_t *item, apr_uint64_t len)
> > {
> > - char readbuf[4096];
> > - apr_size_t readbuf_len;
> > + svn_stringbuf_t *stringbuf;
> >
> > /* We can't store strings longer than the maximum size of apr_size_t,
> > * so check for wrapping */
> > @@ -578,67 +577,36 @@ read_long_string(svn_ra_svn_conn_t *conn
> > return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> > _("String length larger than maximum"));
> >
> > + /* Read the string in chunks. The chunk size is large enough to avoid
> > + * re-allocation in typical cases, and small enough to ensure we do not
> > + * pre-allocate an unreasonable amount of memory if (perhaps due to
> > + * network data corruption or a DOS attack), we receive a bogus claim that
> > + * a very long string is going to follow. In that case, we start small
> > + * and wait for all that data to actually show up. This does not fully
> > + * prevent DOS attacks but makes them harder (you have to actually send
> > + * gigabytes of data). */
> > + stringbuf = svn_stringbuf_create_ensure(
> > + (apr_size_t)(len < SUSPICIOUSLY_HUGE_STRING_SIZE_THRESHOLD
> > + ? len : SUSPICIOUSLY_HUGE_STRING_SIZE_THRESHOLD),
> > + pool);
> > +
>
> Sorry to point this just now, but this change seems to be problematic
> for the common case:
>
> It will make *every* string read allocate 1MB of memory [...]
No it won't. LEN is the expected length of the string being received,
and it will allocate the smaller of LEN and 1MB.
- Julian
Received on 2011-06-08 10:54:25 CEST