+1 to merge to trunk.
Hyrum K. Wright wrote on Wed, Oct 27, 2010 at 16:34:46 -0500: > On Wed, Oct 27, 2010 at 3:40 PM, <[email protected]> wrote: > > Author: stefan2 > > Date: Wed Oct 27 20:40:53 2010 > > New Revision: 1028092 > > > > URL: http://svn.apache.org/viewvc?rev=1028092&view=rev > > Log: > > Incorporate feedback I got on r985606. > > > > * subversion/libsvn_ra_svn/marshal.c > > (SUSPICIOUSLY_HUGE_STRING_SIZE_THRESHOLD): introduce symbolic name > > for an otherwise arbitrary number > > (read_long_string): fix docstring > > (read_string): use symbolic name and explain the rationale behind the > > special case > > > > Modified: > > subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c > > > > Modified: subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c > > URL: > > http://svn.apache.org/viewvc/subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c?rev=1028092&r1=1028091&r2=1028092&view=diff > > ============================================================================== > > --- subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c > > (original) > > +++ subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c Wed > > Oct 27 20:40:53 2010 > > @@ -44,6 +44,12 @@ > > > > #define svn_iswhitespace(c) ((c) == ' ' || (c) == '\n') > > > > +/* If we receive data that *claims* to be followed by a very long string, > > + * we should not trust that claim right away. But everything up to 1 MB > > + * should be too small to be instrumental for a DOS attack. */ > > + > > +#define SUSPICIOUSLY_HUGE_STRING_SIZE_THRESHOLD (0x100000) > > I like the name! > > > + > > /* --- CONNECTION INITIALIZATION --- */ > > > > svn_ra_svn_conn_t *svn_ra_svn_create_conn2(apr_socket_t *sock, > > @@ -555,9 +561,8 @@ svn_error_t *svn_ra_svn_write_tuple(svn_ > > > > /* --- READING DATA ITEMS --- */ > > > > -/* 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. */ > > +/* 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) > > @@ -593,7 +598,14 @@ static svn_error_t *read_string(svn_ra_s > > svn_ra_svn_item_t *item, apr_uint64_t len) > > { > > svn_stringbuf_t *stringbuf; > > - if (len > 0x100000) > > + > > + /* We should not use large strings in our protocol. However, we may > > + * receive a 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 attacs but makes them harder (you > > + * have to actually send gigabytes of data). > > Wow, I hadn't even considered this. Once we get this on trunk, it > might make sense to propose a backport, since this has (potential?) > security implications. > > > + */ > > + if (len > SUSPICIOUSLY_HUGE_STRING_SIZE_THRESHOLD) > > { > > /* This string might take a large amount of memory. Don't allocate > > * the whole buffer at once, so to prevent OOM issues by corrupted > > > > -Hyrum
