Bug 170692

Summary:

B3: don't allow unsigned offsets in MemoryValue

Product:

WebKit

Reporter:

JF Bastien <jfbastien>

Component:

JavaScriptCore

Assignee:

JF Bastien <jfbastien>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates, fpizlo, jfbastien, keith_miller, mark.lam, msaboff, saam

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch	fpizlo: review+, fpizlo: commit-queue-
patch	none
patch	commit-queue: commit-queue-
patch	none

JF Bastien

Reported 2017-04-10 14:09:47 PDT

MemoryValue always expects a signed offset. I ran into this for another patch, and it seems like an unlikely but really bad bug to run into. I'll audit our code and fix MemoryValue as well as other places which use signed integer. They should either fail to compile if given unsigned, or check and trap at runtime if we'd hit implementation-defined behavior.

Attachments
patch (30.73 KB, patch) 2017-04-14 00:20 PDT, JF Bastien	fpizlo: review+ fpizlo: commit-queue-	Details Formatted Diff Diff
patch (61.35 KB, patch) 2017-04-14 17:00 PDT, JF Bastien	no flags	Details Formatted Diff Diff
patch (61.62 KB, patch) 2017-04-17 00:23 PDT, JF Bastien	commit-queue: commit-queue-	Details Formatted Diff Diff
patch (61.61 KB, patch) 2017-04-17 00:26 PDT, JF Bastien	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

JF Bastien

Comment 1 2017-04-14 00:20:40 PDT

Created attachment 307101 [details] patch

Build Bot

Comment 2 2017-04-14 00:23:11 PDT

Attachment 307101 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:174: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:174: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:175: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:175: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:176: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:176: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:176: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 7 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Keith Miller

Comment 3 2017-04-14 09:09:58 PDT

Comment on attachment 307101 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=307101&action=review > Source/JavaScriptCore/b3/B3MemoryValue.cpp:83 > + MemoryValue::MemoryValue(MemoryValue::MemoryValueLoad, Kind kind, Type type, Origin origin, Value* pointer, MemoryValue::OffsetType offset, HeapRange range, HeapRange fenceRange) undo indentation please.

Filip Pizlo

Comment 4 2017-04-14 09:20:46 PDT

Comment on attachment 307101 [details] patch Actually, cq- because I also want you to undo that indentation change in MemoryValue that Keith pointed out.

Filip Pizlo

Comment 5 2017-04-14 09:21:52 PDT

As ugly as this is, I think it's good to force people to be precise about how their offset becomes an int32. I have a question: what does this do to AtomicValue?

JF Bastien

Comment 6 2017-04-14 17:00:45 PDT

Created attachment 307169 [details] patch Here's an update that does AtomicValue and other offsets! I also added C++17's std::conjunction to WTF because this was becoming copy-pasta. The heart of this change is now in B3Value.h.

Build Bot

Comment 7 2017-04-14 17:02:56 PDT

Attachment 307169 [details] did not pass style-queue: ERROR: Source/WTF/wtf/StdLibExtras.h:516: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WTF/wtf/StdLibExtras.h:517: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WTF/wtf/StdLibExtras.h:518: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WTF/wtf/StdLibExtras.h:519: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WTF/wtf/StdLibExtras.h:520: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:88: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:88: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:89: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:89: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/air/AirArg.h:549: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3Value.h:297: std::enable_if::type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/b3/B3Value.h:298: std::enable_if::type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/b3/B3Value.h:300: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:146: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:146: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:147: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:147: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:148: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:148: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:148: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 20 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

JF Bastien

Comment 8 2017-04-17 00:23:35 PDT

Created attachment 307258 [details] patch Fix style and MSVC build.

WebKit Commit Bot

Comment 9 2017-04-17 00:24:08 PDT

Comment on attachment 307258 [details] patch Rejecting attachment 307258 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 307258, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/3549717

JF Bastien

Comment 10 2017-04-17 00:26:47 PDT

Created attachment 307259 [details] patch Forgot to update "oops", as always.

WebKit Commit Bot

Comment 11 2017-04-17 01:24:25 PDT

The commit-queue encountered the following flaky tests while processing attachment 307259 [details]: webrtc/captureCanvas-webrtc.html bug 170870 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 12 2017-04-17 01:24:52 PDT

Comment on attachment 307259 [details] patch Clearing flags on attachment: 307259 Committed r215407: <http://trac.webkit.org/changeset/215407>

WebKit Commit Bot

Comment 13 2017-04-17 01:24:54 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.