Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2263263003:
hunspell: Changed signed char* to unsigned char* (Closed)
|
Created:
4 years, 4 months ago by Kevin Bailey Modified:
4 years, 4 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews, groby+spellwatch_chromium.org, rlp+watch_chromium.org, rouslan+spell_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChanged signed char* to unsigned char*, fuzzer fix
Fuzzer reports undefined behavior precisely at the locations where
it performs a left shift on a signed char. Changing it to a regular
char makes the errors go away.
Since the function is doing lots of bit manipulation, a signed char
seems inappropriate anyways. All spellcheck tests pass with regular
char.
The upstream github history (that I could find) shows that it has
always been signed, so I couldn't find a reason why it changed, if
it ever did.
BUG=620659, 624348
Committed: https://crrev.com/e87c5a27a35c73cc8b099742d914d79d7543deea
Cr-Commit-Position: refs/heads/master@{#414095}
Patch Set 1 #Patch Set 2 : Explicit unsigned and formatting #Patch Set 3 : Updated google.patch #Patch Set 4 : Unformatting #Messages
Total messages: 21 (11 generated)
Description was changed from ========== Changed signed char* to char* Fuzzer reports undefined behavior precisely at the locations where it performs a left shift on a signed char. Changing it to a regular char makes the errors go away. Since the function is doing lots of bit manipulation, a signed char seems inappropriate anyways. All spellcheck tests pass with regular char. The upstream github history (that I could find) shows that it has always been signed, so I couldn't find a reason why it changed, if it ever did. BUG=620659,624348 ========== to ========== Changed signed char* to char* Fuzzer reports undefined behavior precisely at the locations where it performs a left shift on a signed char. Changing it to a regular char makes the errors go away. Since the function is doing lots of bit manipulation, a signed char seems inappropriate anyways. All spellcheck tests pass with regular char. The upstream github history (that I could find) shows that it has always been signed, so I couldn't find a reason why it changed, if it ever did. BUG=620659,624348 ==========
krb@chromium.org changed reviewers: + groby@chromium.org
Description was changed from ========== Changed signed char* to char* Fuzzer reports undefined behavior precisely at the locations where it performs a left shift on a signed char. Changing it to a regular char makes the errors go away. Since the function is doing lots of bit manipulation, a signed char seems inappropriate anyways. All spellcheck tests pass with regular char. The upstream github history (that I could find) shows that it has always been signed, so I couldn't find a reason why it changed, if it ever did. BUG=620659,624348 ========== to ========== Changed signed char* to char*, fuzzer fix Fuzzer reports undefined behavior precisely at the locations where it performs a left shift on a signed char. Changing it to a regular char makes the errors go away. Since the function is doing lots of bit manipulation, a signed char seems inappropriate anyways. All spellcheck tests pass with regular char. The upstream github history (that I could find) shows that it has always been signed, so I couldn't find a reason why it changed, if it ever did. BUG=620659,624348 ==========
In case it hasn't sent an email yet...
On 2016/08/23 15:19:07, Kevin Bailey wrote: > In case it hasn't sent an email yet... Thanks for tackling this! We probably should upstream the change. If you want to keep this downstream, you'll also need to update the google patches for hunspell. (I'd also suggest making it explicitly an unsigned char, so the proper behavior is preserved if somebody somewhere futzes with compile flags)
On 2016/08/23 15:23:58, groby wrote: > On 2016/08/23 15:19:07, Kevin Bailey wrote: > > In case it hasn't sent an email yet... > > Thanks for tackling this! > > We probably should upstream the change. If you want to keep this downstream, > you'll also need to update the google patches for hunspell. Upstream has already moved on. The function doesn't even exist, replaced with a std:string version. I see this change as an interim patch to make fuzzer happy, so I updated google.patch > (I'd also suggest > making it explicitly an unsigned char, so the proper behavior is preserved if > somebody somewhere futzes with compile flags) Done.
On 2016/08/23 16:05:07, Kevin Bailey wrote: > On 2016/08/23 15:23:58, groby wrote: > > On 2016/08/23 15:19:07, Kevin Bailey wrote: > > > In case it hasn't sent an email yet... > > > > Thanks for tackling this! > > > > We probably should upstream the change. If you want to keep this downstream, > > you'll also need to update the google patches for hunspell. > > Upstream has already moved on. The function doesn't even exist, replaced with > a std:string version. I see this change as an interim patch to make fuzzer > happy, so I updated google.patch > > > (I'd also suggest > > making it explicitly an unsigned char, so the proper behavior is preserved if > > somebody somewhere futzes with compile flags) > > Done. I'm terribly sorry to say this, but: Please don't format hunspell. It's third-party, and doesn't follow our standards, so makes for really large patches. Also: Are you signing up for pulling a newer version? :)
On 2016/08/23 17:28:42, groby wrote: > > I'm terribly sorry to say this, but: Please don't format hunspell. It's > third-party, and doesn't follow our standards, so makes for really large > patches. No worries, but I don't know that it helped much. > Also: Are you signing up for pulling a newer version? :) I'll see what rouslan and phadjan want to do, but certifying it would be a much bigger job than this little CL.
On 2016/08/23 18:07:21, Kevin Bailey wrote: > I'll see what rouslan and phadjan want to do, but certifying it would be > a much bigger job than this little CL. +1 to updating to latest hunspell. Understood that it can happen in a separate CL.
Description was changed from ========== Changed signed char* to char*, fuzzer fix Fuzzer reports undefined behavior precisely at the locations where it performs a left shift on a signed char. Changing it to a regular char makes the errors go away. Since the function is doing lots of bit manipulation, a signed char seems inappropriate anyways. All spellcheck tests pass with regular char. The upstream github history (that I could find) shows that it has always been signed, so I couldn't find a reason why it changed, if it ever did. BUG=620659,624348 ========== to ========== Changed signed char* to unsigned char*, fuzzer fix Fuzzer reports undefined behavior precisely at the locations where it performs a left shift on a signed char. Changing it to a regular char makes the errors go away. Since the function is doing lots of bit manipulation, a signed char seems inappropriate anyways. All spellcheck tests pass with regular char. The upstream github history (that I could find) shows that it has always been signed, so I couldn't find a reason why it changed, if it ever did. BUG=620659,624348 ==========
On 2016/08/23 18:08:45, rouslan wrote: > On 2016/08/23 18:07:21, Kevin Bailey wrote: > > I'll see what rouslan and phadjan want to do, but certifying it would be > > a much bigger job than this little CL. > > +1 to updating to latest hunspell. Understood that it can happen in a separate > CL. LGTM
The CQ bit was checked by krb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by krb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Changed signed char* to unsigned char*, fuzzer fix Fuzzer reports undefined behavior precisely at the locations where it performs a left shift on a signed char. Changing it to a regular char makes the errors go away. Since the function is doing lots of bit manipulation, a signed char seems inappropriate anyways. All spellcheck tests pass with regular char. The upstream github history (that I could find) shows that it has always been signed, so I couldn't find a reason why it changed, if it ever did. BUG=620659,624348 ========== to ========== Changed signed char* to unsigned char*, fuzzer fix Fuzzer reports undefined behavior precisely at the locations where it performs a left shift on a signed char. Changing it to a regular char makes the errors go away. Since the function is doing lots of bit manipulation, a signed char seems inappropriate anyways. All spellcheck tests pass with regular char. The upstream github history (that I could find) shows that it has always been signed, so I couldn't find a reason why it changed, if it ever did. BUG=620659,624348 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Changed signed char* to unsigned char*, fuzzer fix Fuzzer reports undefined behavior precisely at the locations where it performs a left shift on a signed char. Changing it to a regular char makes the errors go away. Since the function is doing lots of bit manipulation, a signed char seems inappropriate anyways. All spellcheck tests pass with regular char. The upstream github history (that I could find) shows that it has always been signed, so I couldn't find a reason why it changed, if it ever did. BUG=620659,624348 ========== to ========== Changed signed char* to unsigned char*, fuzzer fix Fuzzer reports undefined behavior precisely at the locations where it performs a left shift on a signed char. Changing it to a regular char makes the errors go away. Since the function is doing lots of bit manipulation, a signed char seems inappropriate anyways. All spellcheck tests pass with regular char. The upstream github history (that I could find) shows that it has always been signed, so I couldn't find a reason why it changed, if it ever did. BUG=620659,624348 Committed: https://crrev.com/e87c5a27a35c73cc8b099742d914d79d7543deea Cr-Commit-Position: refs/heads/master@{#414095} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e87c5a27a35c73cc8b099742d914d79d7543deea Cr-Commit-Position: refs/heads/master@{#414095} |
