Bug 170692 – B3: don't allow unsigned offsets in MemoryValue

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

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.

[JF Bastien]

Created attachment 307101 [details]
patch

[Build Bot]

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 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 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]

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]

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]

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]

Created attachment 307258 [details]
patch

Fix style and MSVC build.

[WebKit Commit Bot]

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]

Created attachment 307259 [details]
patch

Forgot to update "oops", as always.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 307259 [details]:

webrtc/captureCanvas-webrtc.html bug 170870 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 307259 [details]
patch

Clearing flags on attachment: 307259

Committed r215407: <http://trac.webkit.org/changeset/215407>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.