[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1028092 - /subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c

From: Hyrum K. Wright <hyrum_wright_at_mail.utexas.edu>
Date: Wed, 27 Oct 2010 16:34:46 -0500

On Wed, Oct 27, 2010 at 3:40 PM, <stefan2_at_apache.org> 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
Received on 2010-10-27 23:35:27 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.