213383 2020-06-19 09:23:00 -0700 -Wsign-compare in isValidOptionSet 2020-06-22 14:44:07 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build PC Linux RESOLVED FIXED InRadar P2 Minor --- 1 mcatanzaro mcatanzaro benjamin cdumez cmarcelo darin ddkilzer ews-watchlist mcatanzaro webkit-bug-importer oldest_to_newest 1664389 0 mcatanzaro 2020-06-19 09:23:00 -0700 Since r263208 "[IPC hardening] OptionSet<> values should be validated": [4332/4574] Building CXX object Source...ources/UnifiedSource-50d0d8dd-14.cpp.o In file included from /home/mcatanzaro/Projects/WebKit/Source/WebKit/Shared/glib/InputMethodState.h:31, from /home/mcatanzaro/Projects/WebKit/Source/WebKit/Shared/glib/InputMethodState.cpp:27, from DerivedSources/WebKit/unified-sources/UnifiedSource-50d0d8dd-14.cpp:1: DerivedSources/ForwardingHeaders/wtf/OptionSet.h: In instantiation of ‘constexpr bool WTF::isValidOptionSet(WTF::OptionSet<E>) [with E = WebKit::InputMethodState::Hint]’: /home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/ArgumentCoders.h:69:35: required from ‘static bool IPC::ArgumentCoder<WTF::OptionSet<E> >::decode(IPC::Decoder&, WTF::OptionSet<E>&) [with T = WebKit::InputMethodState::Hint]’ /home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/ArgumentCoder.h:72:41: required by substitution of ‘template<class T> static uint8_t IPC::UsesLegacyDecoder<WTF::OptionSet<WebKit::InputMethodState::Hint> >::checkArgumentCoder<T>(IPC::UsesLegacyDecoder<WTF::OptionSet<WebKit::InputMethodState::Hint> >::Helper<bool (*)(IPC::Decoder&, WTF::OptionSet<WebKit::InputMethodState::Hint>&), (& IPC::ArgumentCoder<T>::decode)>*) [with T = WTF::OptionSet<WebKit::InputMethodState::Hint>]’ /home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/ArgumentCoder.h:76:85: required from ‘constexpr const bool IPC::UsesLegacyDecoder<WTF::OptionSet<WebKit::InputMethodState::Hint> >::value’ /home/mcatanzaro/Projects/WebKit/Source/WebKit/Platform/IPC/Decoder.h:140:126: required by substitution of ‘template<class T, std::enable_if_t<(((! std::is_enum<_Tp>::value) && (! std::is_arithmetic<_Tp>::value)) && (! IPC::UsesLegacyDecoder<U>::value))>* <anonymous> > bool IPC::Decoder::decode(T&) [with T = WTF::OptionSet<WebKit::InputMethodState::Hint>; std::enable_if_t<(((! std::is_enum<_Tp>::value) && (! std::is_arithmetic<_Tp>::value)) && (! IPC::UsesLegacyDecoder<U>::value))>* <anonymous> = <missing>]’ /home/mcatanzaro/Projects/WebKit/Source/WebKit/Shared/glib/InputMethodState.cpp:117:36: required from here DerivedSources/ForwardingHeaders/wtf/OptionSet.h:257:52: warning: comparison of integer expressions of different signedness: ‘WTF::OptionSet<WebKit::InputMethodState::Hint>::StorageType’ {aka ‘unsigned int’} and ‘int’ [-Wsign-compare] 257 | return (optionSet.toRaw() | allValidBitsValue) == allValidBitsValue; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~ 1664394 1 mcatanzaro 2020-06-19 09:35:11 -0700 Since this is a bitwise comparison, and we don't normally want to use signed types for bitwise comparisons, we probably want to cast allValidBitsValue on the right side of the expression to unsigned. I think that can be done using std::make_unsigned. Testing this.... 1664415 2 mcatanzaro 2020-06-19 10:46:26 -0700 So that does work, but I'm going to try to mandate use of unsigned type instead. Using signed types with OptionSet is probably a mistake. 1664429 3 mcatanzaro 2020-06-19 11:21:11 -0700 (In reply to Michael Catanzaro from comment #2) > So that does work, but I'm going to try to mandate use of unsigned type > instead. Using signed types with OptionSet is probably a mistake. So that was a bad idea, because OptionSet goes out of its way to allow this: using StorageType = std::make_unsigned_t<std::underlying_type_t<E>>; I think isValidOptionSet should do the same. Let me try that. 1664445 4 mcatanzaro 2020-06-19 11:57:40 -0700 OK, I present two... options. ;) Option #1: OptionSetValueChecker should match the OptionSet's StorageType, which is std::make_unsigned_t<std::underlying_type_t<E>>, instead of directly using std::underlying_type_t<E>: diff --git a/Source/WTF/wtf/OptionSet.h b/Source/WTF/wtf/OptionSet.h index de111b16b355..8450b1a240d9 100644 --- a/Source/WTF/wtf/OptionSet.h +++ b/Source/WTF/wtf/OptionSet.h @@ -253,8 +253,8 @@ private: template<typename E> WARN_UNUSED_RETURN constexpr bool isValidOptionSet(OptionSet<E> optionSet) { - auto allValidBitsValue = OptionSetValueChecker<std::underlying_type_t<E>, typename EnumTraits<E>::values>::allValidBits(); - return (optionSet.toRaw() | allValidBitsValue) == allValidBitsValue; + auto allValidBitsValue = OptionSetValueChecker<std::make_unsigned_t<std::underlying_type_t<E>>, typename EnumTraits<E>::values>::allValidBits(); + return (optionSet.toRaw() | allValidBitsValue) == std::underlying_type_t<E>(allValidBitsValue); } That fixes the warning without making any changes in other files. I'll attach this patch for review. Option #2: we could require the OptionSet to use an unsigned underlying value to call isValidOptionSet using std::enable_if. That would enforce unsigned underlying value only for OptionSets that are validated by isValidOptionSet. We only have to change the underlying type of WebKit::InputMethodState::Hint in Source/WebKit/Shared/glib/InputMethodState.h. I decided against making this my preferred option because valid OptionSets do not actually have to use unsigned underlying storage, so it would be a bit weird to require that to call isValidOptionSet, but this would be useful if we want to enforce this for types sent over IPC. (Do we want to enforce it for types sent over IPC?) Option #3: would be to prohibit OptionSet from ever using signed underlying value, but that's going to require changing a lot of enums. Doesn't seem worth it, so I don't propose this. 1664447 5 mcatanzaro 2020-06-19 12:00:07 -0700 (In reply to Michael Catanzaro from comment #4) > + return (optionSet.toRaw() | allValidBitsValue) == > std::underlying_type_t<E>(allValidBitsValue); That's a Ctrl+Z mistake, I'll attach a patch that actually works. 1664448 6 402301 mcatanzaro 2020-06-19 12:00:23 -0700 Created attachment 402301 Patch 1664451 7 mcatanzaro 2020-06-19 12:06:48 -0700 (In reply to Michael Catanzaro from comment #4) > this would be useful if we want to enforce this > for types sent over IPC. (Do we want to enforce it for types sent over IPC?) Maybe we do? r263208 ensured this for all types used in cross-platform code. If it's important, r- my patch and I'll upload a version that enforces this. The downside is that it would no longer be possible to call isValidOptionSet on valid OptionSets that use signed underlying storage, which would be weird. 1664465 8 ews-feeder 2020-06-19 12:52:27 -0700 Committed r263280: <https://trac.webkit.org/changeset/263280> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402301. 1664467 9 webkit-bug-importer 2020-06-19 12:53:19 -0700 <rdar://problem/64541629> 1665185 10 ddkilzer 2020-06-22 14:44:07 -0700 Thanks Michael! 402301 2020-06-19 12:00:23 -0700 2020-06-19 12:52:27 -0700 Patch bug-213383-20200619140022.patch text/plain 1562 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjYzMjY4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGIxYjg1YzFjMGY0OTllNDdlZTc5OWU2 YTRkNTlhMzIwMDc5NTViNDEuLjFiNDFhYTc4ZDY3MGNlYTJjNzljOGFlZjM4NmI4ZmYyZWM4N2Zi YTYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMjAtMDYtMTkgIE1pY2hhZWwgQ2F0YW56YXJvICA8 bWNhdGFuemFyb0Bnbm9tZS5vcmc+CisKKyAgICAgICAgLVdzaWduLWNvbXBhcmUgaW4gaXNWYWxp ZE9wdGlvblNldAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MjEzMzgzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgVGhlIE9wdGlvblNldCdzIFN0b3JhZ2VUeXBlIGlzIGFsd2F5cyB1bnNpZ25lZCwgZXZlbiBp ZiB0aGUgZW51bSdzIHVuZGVybHlpbmcgdmFsdWUgaXMgbm90LgorICAgICAgICBNYXRjaCB0aGlz IGluIGlzVmFsaWRPcHRpb25TZXQgdG8gYXZvaWQgLVdzaWduLWNvbXBhcmUgZHVyaW5nIHZhbGlk aXR5IGNoZWNraW5nLgorCisgICAgICAgICogd3RmL09wdGlvblNldC5oOgorICAgICAgICAoV1RG Ojppc1ZhbGlkT3B0aW9uU2V0KToKKwogMjAyMC0wNi0xOSAgTXlsZXMgQy4gTWF4ZmllbGQgIDxt bWF4ZmllbGRAYXBwbGUuY29tPgogCiAgICAgICAgIFtDb2NvYV0gVW5pZnkgImZvbnQ6IiBDU1Mg c2hvcnRoYW5kIHZhbHVlcyBiZXR3ZWVuIG1hY09TIGFuZCBpT1MgZmFtaWx5CmRpZmYgLS1naXQg YS9Tb3VyY2UvV1RGL3d0Zi9PcHRpb25TZXQuaCBiL1NvdXJjZS9XVEYvd3RmL09wdGlvblNldC5o CmluZGV4IGRlMTExYjE2YjM1NTI2ODJjMTFjYjJkMTc0N2QxZDMwN2QyNzhmOGUuLmQ5N2NjN2M0 MGMyZTcyMzhiM2RjYzc0NzVhNmE5ODEzYzE4MWZmNTkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYv d3RmL09wdGlvblNldC5oCisrKyBiL1NvdXJjZS9XVEYvd3RmL09wdGlvblNldC5oCkBAIC0yNTMs NyArMjUzLDcgQEAgcHJpdmF0ZToKIHRlbXBsYXRlPHR5cGVuYW1lIEU+CiBXQVJOX1VOVVNFRF9S RVRVUk4gY29uc3RleHByIGJvb2wgaXNWYWxpZE9wdGlvblNldChPcHRpb25TZXQ8RT4gb3B0aW9u U2V0KQogewotICAgIGF1dG8gYWxsVmFsaWRCaXRzVmFsdWUgPSBPcHRpb25TZXRWYWx1ZUNoZWNr ZXI8c3RkOjp1bmRlcmx5aW5nX3R5cGVfdDxFPiwgdHlwZW5hbWUgRW51bVRyYWl0czxFPjo6dmFs dWVzPjo6YWxsVmFsaWRCaXRzKCk7CisgICAgYXV0byBhbGxWYWxpZEJpdHNWYWx1ZSA9IE9wdGlv blNldFZhbHVlQ2hlY2tlcjxzdGQ6Om1ha2VfdW5zaWduZWRfdDxzdGQ6OnVuZGVybHlpbmdfdHlw ZV90PEU+PiwgdHlwZW5hbWUgRW51bVRyYWl0czxFPjo6dmFsdWVzPjo6YWxsVmFsaWRCaXRzKCk7 CiAgICAgcmV0dXJuIChvcHRpb25TZXQudG9SYXcoKSB8IGFsbFZhbGlkQml0c1ZhbHVlKSA9PSBh bGxWYWxpZEJpdHNWYWx1ZTsKIH0KIAo=