Bug 39327 - Enforce size constraints on various data structures in JavaScriptCore/wtf.
Summary: Enforce size constraints on various data structures in JavaScriptCore/wtf.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-18 16:00 PDT by David Levin
Modified: 2010-09-02 13:48 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.42 KB, patch)
2010-05-18 16:05 PDT, David Levin
no flags Details | Formatted Diff | Diff
Proposed fix (same as last time, just updated to not have conflicts with things that have landed). (9.18 KB, patch)
2010-05-21 14:07 PDT, David Levin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2010-05-18 16:00:05 PDT
Enforce size constraints on various data structures in JavaScriptCore/wtf.
Comment 1 David Levin 2010-05-18 16:05:55 PDT
Created attachment 56418 [details]
Patch
Comment 2 David Levin 2010-05-18 16:10:23 PDT
This is a start of helping to automatically enforce memory constraints on key data structures.

The idea for doing this comes from https://bugs.webkit.org/show_bug.cgi?id=38906#c5 and that it would be good to detect issues like this automatically at build time.

This patch only addresses wtf items. (A future one could address some WebCore items.)
Comment 3 David Levin 2010-05-21 14:07:07 PDT
Created attachment 56744 [details]
Proposed fix (same as last time, just updated to not have conflicts with things that have landed).
Comment 4 WebKit Review Bot 2010-05-21 14:08:45 PDT
Attachment 56744 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/wtf/WTFCompileAsserts.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 David Levin 2010-05-21 14:14:34 PDT
(In reply to comment #4)
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> JavaScriptCore/wtf/WTFCompileAsserts.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
> Total errors found: 1 in 6 files
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Done: https://bugs.webkit.org/show_bug.cgi?id=39514
Comment 6 Darin Adler 2010-05-21 15:26:48 PDT
This seems like a great idea. Anders Carlsson and I have talked about going even further.
Comment 7 Darin Adler 2010-05-21 15:28:05 PDT
Comment on attachment 56744 [details]
Proposed fix (same as last time, just updated to not have conflicts with things that have landed).

> +++ b/JavaScriptCore/wtf/WTFCompileAsserts.cpp

It seems strange to have WTF in the filename here. Also, I think they are assertions, not "asserts".

If the intent here is to have these be size assertions only, then I think the file name is not quite right.
Comment 8 David Levin 2010-05-21 15:45:04 PDT
(In reply to comment #7)
> (From update of attachment 56744 [details])
> > +++ b/JavaScriptCore/wtf/WTFCompileAsserts.cpp
> 
> It seems strange to have WTF in the filename here. Also, I think they are assertions, not "asserts".
> 
> If the intent here is to have these be size assertions only, then I think the file name is not quite right.

How about SizeLimits.cpp ?

This is a cpp file which contains size constraints/limits for data structures in WTF (that don't have their own cpp file).
Comment 9 Darin Adler 2010-05-21 15:45:39 PDT
(In reply to comment #8)
> How about SizeLimits.cpp ?

Sounds pretty good to me.
Comment 10 WebKit Review Bot 2010-05-21 16:11:33 PDT
http://trac.webkit.org/changeset/59969 might have broken Chromium Linux Release
Comment 11 David Levin 2010-05-21 16:20:51 PDT
(In reply to comment #10)
> http://trac.webkit.org/changeset/59969 might have broken Chromium Linux Release

Fix this and Snow Leopard, here http://trac.webkit.org/changeset/59970 (by removing one compile assert for the moment for cross thread ref counted -- I'll look to add this one back in the future).
Comment 12 Eric Seidel (no email) 2010-09-02 13:48:53 PDT
Looks like this can be closed.