You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2010/10/27 22:40:53 UTC
svn commit: r1028092 -
/subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c
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)
+
/* --- 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).
+ */
+ 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
Re: svn commit: r1028092 -
/subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
+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, <st...@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
Re: svn commit: r1028092 -
/subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
+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, <st...@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
Re: SVN_PROP_MAX_SIZE (was: Re: svn commit: r1028092 -
/subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c)
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Oct 28, 2010 at 02:35:49PM +0100, Julian Foad wrote:
> On Thu, 2010-10-28 at 14:22 +0200, Stefan Sperling wrote:
> > Can we define a constant like SVN_PROP_MAX_SIZE that says how large
> > a property can be? This would be useful in the patch code, for instance.
> > Right now, a patch with a large property value will cause out of memory
> > errors because the code has no way of knowing the maximum size of a
> > property value -- so it just keeps reading and allocating.
> >
> > Also, do we have a plan for what to do when (not "if") mergeinfo in
> > repositories out there hits the limit?
>
> Hi Stefan. I thought the issue being addressed here was not imposing a
> limit on property size, but rather just limiting the allocation of an
> initial chunk of memory which can still be expanded later if the claimed
> amount of data actually arrives.
>
> "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)."
>
> "Don't allocate the whole buffer at once ..."
Ah, I misunderstood. Thanks!
Stefan
Re: SVN_PROP_MAX_SIZE (was: Re: svn commit: r1028092 -
/subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c)
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2010-10-28 at 14:22 +0200, Stefan Sperling wrote:
> On Thu, Oct 28, 2010 at 09:30:49AM +0200, Erik Huelsmann wrote:
> > >> @@ -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.
> >
> >
> > Actually, that was already released as a security vulnerability some
> > years ago. The comment by Stefan makes it painfully apparent that it
> > is, but I guess that's a good thing. Notice that he did nothing but
> > name the constant and add the explanation. See
> > http://subversion.apache.org/security/CAN-2004-0413-advisory.txt
> >
> > This is exactly the point I was talking about when I said properties
> > are length-limited by ra_svn. (In relation to the maximum size of
> > merge-tracking information.) The actual code is a little bit
> > different than what I remembered., because it does seem to grow the
> > buffer once it gets past the first MiB.
>
> Can we define a constant like SVN_PROP_MAX_SIZE that says how large
> a property can be? This would be useful in the patch code, for instance.
> Right now, a patch with a large property value will cause out of memory
> errors because the code has no way of knowing the maximum size of a
> property value -- so it just keeps reading and allocating.
>
> Also, do we have a plan for what to do when (not "if") mergeinfo in
> repositories out there hits the limit?
Hi Stefan. I thought the issue being addressed here was not imposing a
limit on property size, but rather just limiting the allocation of an
initial chunk of memory which can still be expanded later if the claimed
amount of data actually arrives.
"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)."
"Don't allocate the whole buffer at once ..."
- Julian
SVN_PROP_MAX_SIZE (was: Re: svn commit: r1028092 -
/subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c)
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Oct 28, 2010 at 09:30:49AM +0200, Erik Huelsmann wrote:
> >> @@ -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.
>
>
> Actually, that was already released as a security vulnerability some
> years ago. The comment by Stefan makes it painfully apparent that it
> is, but I guess that's a good thing. Notice that he did nothing but
> name the constant and add the explanation. See
> http://subversion.apache.org/security/CAN-2004-0413-advisory.txt
>
> This is exactly the point I was talking about when I said properties
> are length-limited by ra_svn. (In relation to the maximum size of
> merge-tracking information.) The actual code is a little bit
> different than what I remembered., because it does seem to grow the
> buffer once it gets past the first MiB.
Can we define a constant like SVN_PROP_MAX_SIZE that says how large
a property can be? This would be useful in the patch code, for instance.
Right now, a patch with a large property value will cause out of memory
errors because the code has no way of knowing the maximum size of a
property value -- so it just keeps reading and allocating.
Also, do we have a plan for what to do when (not "if") mergeinfo in
repositories out there hits the limit?
Stefan
SVN_PROP_MAX_SIZE (was: Re: svn commit: r1028092 -
/subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c)
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Oct 28, 2010 at 09:30:49AM +0200, Erik Huelsmann wrote:
> >> @@ -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.
>
>
> Actually, that was already released as a security vulnerability some
> years ago. The comment by Stefan makes it painfully apparent that it
> is, but I guess that's a good thing. Notice that he did nothing but
> name the constant and add the explanation. See
> http://subversion.apache.org/security/CAN-2004-0413-advisory.txt
>
> This is exactly the point I was talking about when I said properties
> are length-limited by ra_svn. (In relation to the maximum size of
> merge-tracking information.) The actual code is a little bit
> different than what I remembered., because it does seem to grow the
> buffer once it gets past the first MiB.
Can we define a constant like SVN_PROP_MAX_SIZE that says how large
a property can be? This would be useful in the patch code, for instance.
Right now, a patch with a large property value will cause out of memory
errors because the code has no way of knowing the maximum size of a
property value -- so it just keeps reading and allocating.
Also, do we have a plan for what to do when (not "if") mergeinfo in
repositories out there hits the limit?
Stefan
Re: svn commit: r1028092 - /subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c
Posted by Erik Huelsmann <eh...@gmail.com>.
Hi Hyrum,
On Wed, Oct 27, 2010 at 11:34 PM, Hyrum K. Wright
<hy...@mail.utexas.edu> wrote:
> On Wed, Oct 27, 2010 at 3:40 PM, <st...@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.
Actually, that was already released as a security vulnerability some
years ago. The comment by Stefan makes it painfully apparent that it
is, but I guess that's a good thing. Notice that he did nothing but
name the constant and add the explanation. See
http://subversion.apache.org/security/CAN-2004-0413-advisory.txt
This is exactly the point I was talking about when I said properties
are length-limited by ra_svn. (In relation to the maximum size of
merge-tracking information.) The actual code is a little bit
different than what I remembered., because it does seem to grow the
buffer once it gets past the first MiB.
Regards,
Erik.
Re: svn commit: r1028092 - /subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Wed, Oct 27, 2010 at 3:40 PM, <st...@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
Re: svn commit: r1028092 - /subversion/branches/performance/subversion/libsvn_ra_svn/marshal.c
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Wed, Oct 27, 2010 at 3:40 PM, <st...@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