[WK2] Drop use of fixed-width types for size values in ArgumentCoder specializations #8781

zdobersek · 2023-01-18T19:37:54Z

`d5514af`

[WK2] Drop use of fixed-width types for size values in ArgumentCoder specializations
https://bugs.webkit.org/show_bug.cgi?id=250777

Reviewed by Kimmo Kinnunen.

Instead of finding better or worse fixed-sized-type matches for size values in
different ArgumentCoder specializations, the types of those size values should
be used. This means simply encoding values of size_t or unsigned types.

The target type of the size value should be noted in the code, just to provide
the necessary context of what is being encoded and have it match the decoding
side. This means the size type is explicitly used either in any temporary
variable where the value is stored, or an additional static_cast is used to
explicitly specify the type when the value is passed to the stream insertion
operator.

This still leaves open the possibility of using mismatched types, i.e. the
size value type not matching what the encoding enforces. Some sort of validation
could be implemented in the encoder class to make sure no undesired narrowing
conversions occur.

* Source/WebKit/Platform/IPC/ArgumentCoders.cpp:
(IPC::ArgumentCoder<CString>::encode):
(IPC::ArgumentCoder<CString>::decode):
(IPC::ArgumentCoder<String>::encode):
(IPC::decodeStringText):
(IPC::ArgumentCoder<String>::decode):
(IPC::ArgumentCoder<StringView>::encode):
* Source/WebKit/Platform/IPC/ArgumentCoders.h:

Canonical link: https://commits.webkit.org/259488@main

e44e2be

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-01-18T19:39:31Z

EWS run on previous version of this PR (hash 9ae8312)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	💥 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
	✅ 🛠 watch-sim

kkinnunen-apple · 2023-01-18T19:57:47Z

Source/WebKit/Platform/IPC/ArgumentCoders.cpp

        return;
    }

-    encoder << static_cast<uint32_t>(string.length());
+    encoder << validateType<size_t>(string.length());


encode.template encode<size_t>(string.length());

Which is more appropriate? I don't know..

Idea was to have compile-time validation of what the expected type returned by the size accessor is. This approach does it through a templated pass-through, avoiding adding static asserts everywhere.

Along with compile-time validation, it also specifies clearly the type that's going into the encoder, giving clear indication of the type that's expected on the decoding side. Just extra assurance against any future changes that might unknowingly end up tweaking these parts.

Yeah, that part is good.
What I mean is that there's an option:
Either:
a) validate via encoder << validateType<size_t>(...)
Or:
b) enforce via encode.template encode<size_t>(...)
Or:
c) enforce via encoder << static_cast<size_t>(..)

So the question is why validate instead of enforce?

If the enforce would be missing, in that case the validate would be missing aswell, so there's no additional security guarantee ?
Maybe the benefit of validate would be if there are warnings of redundant static_casts?
Or that we think that a developer will remove the redundant static cast?

Validating, as done here, forces adjustment to the type that's returned by the size accessor, no unwitting conversion to anything else allowed. It's a bit clunky to work with, and as seen it doesn't really cause anything to be fixed at the moment.

Enforcing can, on integer types, cause problematic conversions. But at the moment I think that's not happening in any of these specializations, so I would just switch to that. b) seems nicest, and it allows to eventually implement validation in a more refined way.

I thought encoder.template encoder<T>() exists, but it doesn't. Can be added later. Going with c).

webkit-early-warning-system · 2023-01-23T21:20:22Z

EWS run on current version of this PR (hash e44e2be)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk2
	✅ 🛠 tv-sim	❌ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge	✅ 🛠 watch-sim

…specializations https://bugs.webkit.org/show_bug.cgi?id=250777 Reviewed by Kimmo Kinnunen. Instead of finding better or worse fixed-sized-type matches for size values in different ArgumentCoder specializations, the types of those size values should be used. This means simply encoding values of size_t or unsigned types. The target type of the size value should be noted in the code, just to provide the necessary context of what is being encoded and have it match the decoding side. This means the size type is explicitly used either in any temporary variable where the value is stored, or an additional static_cast is used to explicitly specify the type when the value is passed to the stream insertion operator. This still leaves open the possibility of using mismatched types, i.e. the size value type not matching what the encoding enforces. Some sort of validation could be implemented in the encoder class to make sure no undesired narrowing conversions occur. * Source/WebKit/Platform/IPC/ArgumentCoders.cpp: (IPC::ArgumentCoder<CString>::encode): (IPC::ArgumentCoder<CString>::decode): (IPC::ArgumentCoder<String>::encode): (IPC::decodeStringText): (IPC::ArgumentCoder<String>::decode): (IPC::ArgumentCoder<StringView>::encode): * Source/WebKit/Platform/IPC/ArgumentCoders.h: Canonical link: https://commits.webkit.org/259488@main

webkit-commit-queue · 2023-01-27T16:36:02Z

Committed 259488@main (d5514af): https://commits.webkit.org/259488@main

Reviewed commits have been landed. Closing PR #8781 and removing active labels.

zdobersek self-assigned this Jan 18, 2023

zdobersek added the WebKit2 Bugs relating to the WebKit2 API layer label Jan 18, 2023

kkinnunen-apple reviewed Jan 18, 2023

View reviewed changes

zdobersek force-pushed the eng/WK2-Drop-use-of-fixed-width-types-for-size-values-in-ArgumentCoder-specializations branch from 9ae8312 to e44e2be Compare January 23, 2023 21:20

zdobersek marked this pull request as ready for review January 23, 2023 21:20

zdobersek requested a review from cdumez as a code owner January 23, 2023 21:20

zdobersek requested a review from kkinnunen-apple January 23, 2023 21:20

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 24, 2023

kkinnunen-apple approved these changes Jan 27, 2023

View reviewed changes

zdobersek added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Jan 27, 2023

webkit-commit-queue force-pushed the eng/WK2-Drop-use-of-fixed-width-types-for-size-values-in-ArgumentCoder-specializations branch from e44e2be to d5514af Compare January 27, 2023 16:35

webkit-commit-queue merged commit d5514af into WebKit:main Jan 27, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WK2] Drop use of fixed-width types for size values in ArgumentCoder specializations #8781

[WK2] Drop use of fixed-width types for size values in ArgumentCoder specializations #8781

zdobersek commented Jan 18, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 18, 2023 •

edited

kkinnunen-apple Jan 18, 2023

zdobersek Jan 19, 2023

kkinnunen-apple Jan 23, 2023

zdobersek Jan 23, 2023

zdobersek Jan 23, 2023

webkit-early-warning-system commented Jan 23, 2023 •

edited

webkit-commit-queue commented Jan 27, 2023

[WK2] Drop use of fixed-width types for size values in ArgumentCoder specializations #8781

[WK2] Drop use of fixed-width types for size values in ArgumentCoder specializations #8781

Conversation

zdobersek commented Jan 18, 2023 • edited by webkit-early-warning-system

d5514af

webkit-early-warning-system commented Jan 18, 2023 • edited

kkinnunen-apple Jan 18, 2023

Choose a reason for hiding this comment

zdobersek Jan 19, 2023

Choose a reason for hiding this comment

kkinnunen-apple Jan 23, 2023

Choose a reason for hiding this comment

zdobersek Jan 23, 2023

Choose a reason for hiding this comment

zdobersek Jan 23, 2023

Choose a reason for hiding this comment

webkit-early-warning-system commented Jan 23, 2023 • edited

webkit-commit-queue commented Jan 27, 2023

zdobersek commented Jan 18, 2023 •

edited by webkit-early-warning-system

`d5514af`

webkit-early-warning-system commented Jan 18, 2023 •

edited

webkit-early-warning-system commented Jan 23, 2023 •

edited