[webkit-reviews] review denied: [Bug 15123] Self-replicating code makes Safari hang and eventually crash : [Attachment 79006] defense against infinite recursion in document.write( )

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied chris reiss
<christopher.reiss at nokia.com>'s request for review:
Bug 15123: Self-replicating code makes Safari hang and eventually crash
https://bugs.webkit.org/show_bug.cgi?id=15123

Attachment 79006: defense against infinite recursion in document.write( )
https://bugs.webkit.org/attachment.cgi?id=79006&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79006&action=review

> WebCore/dom/Document.cpp:214
> +#define MAX_WRITE_RECURSION_DEPTH 20

We usually use constants instead of pre-processor macros.

> WebCore/dom/Document.cpp:425
> +    , m_tooDeepWriteRecursion(false)
> +    , m_writeRecursionDepth(0)

Why do we have both of these variables?  One would seem to be sufficient.

> WebCore/dom/Document.cpp:2222
> +    // Prevent DOS attack from self-replicating javascript which causes
infinite recursion here

This comment isn't necessary.

> WebCore/dom/Document.cpp:2233
> +    if (m_tooDeepWriteRecursion) {
> +	  m_writeRecursionDepth--;
> +	  return;
> +    } else {
> +	  m_writeRecursionDepth++;
> +    }

This logic doesn't make much sense to me.  Typically for these recursion-depth
counts, we use a small class that increments the counter on construction and
decrements the counter on destruction.	That way we know that we never leak a
count.	There are examples of how to do this in the HTML parser related to the
script nesting level.