Skip to content

Check string size before Win32 MultiByte <-> WideChar conversions #94608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
tgucio wants to merge 0 commits into from
Closed

Check string size before Win32 MultiByte <-> WideChar conversions #94608

tgucio wants to merge 0 commits into from

Conversation

@tgucio
Copy link
Contributor

@tgucio tgucio commented Dec 3, 2021 •
edited
Loading

This PR adds exception handling in C++ string resize() before calling WideCharToMultiByte / MultiByteToWideChar.

This is to avoid buffer overruns should resize() call fail (e.g. alloc error or too long a string length). As per C++ standard: "Strong guarantee: if an exception is thrown, there are no changes in the string".

According to Win32 API docs (WideCharToMultiByte, MultiByteToWideChar) it's the caller responsibility to make sure the buffers are correctly allocated.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Dec 3, 2021
@flutter-dashboard
Copy link

flutter-dashboard bot commented Dec 3, 2021

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@google-cla google-cla bot added the cla: yes label Dec 3, 2021
@tgucio tgucio mentioned this pull request Dec 12, 2021
Closed
8 tasks
@tgucio tgucio changed the title Check string size before Win32 MultiByte <-> WideChar conversions Handle exceptions in C++ string resize before Win32 MultiByte <-> WideChar conversions Dec 12, 2021
@christopherfujino
Copy link
Contributor

christopherfujino commented Jan 14, 2022

@cbracken is this a reasonable change?

@christopherfujino christopherfujino added the a: desktop Running on desktop label Jan 14, 2022
@gspencergoog gspencergoog requested a review from cbracken January 20, 2022 22:47
cbracken
cbracken requested changes Jan 28, 2022
View reviewed changes
@tgucio tgucio changed the title Handle exceptions in C++ string resize before Win32 MultiByte <-> WideChar conversions Check string size before Win32 MultiByte <-> WideChar conversions Feb 14, 2022
@flutter-dashboard
Copy link

flutter-dashboard bot commented Feb 14, 2022

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@tgucio tgucio requested a review from cbracken February 14, 2022 14:00
@christopherfujino christopherfujino removed the tool Affects the "flutter" command-line tool. See also t: labels. label Feb 24, 2022
cbracken
cbracken approved these changes Mar 4, 2022
View reviewed changes
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! Thanks for the fix.

@cbracken cbracken closed this Mar 8, 2022
@cbracken cbracken mentioned this pull request Mar 8, 2022
Merged
8 tasks
@cbracken
Copy link
Member

cbracken commented Mar 8, 2022

Unfortunately while attempting to rebase this and re-trigger tests, the update to the @tgucio's original repo failed due to a permissioning error. I've re-opened your patch in #99729, rebased to tip of tree.

@tgucio tgucio mentioned this pull request Aug 2, 2022
Merged
11 tasks
cbracken pushed a commit to flutter/plugins that referenced this pull request Aug 4, 2022
@tgucio
[camera_windows] Check string size before Win32 MultiByte <-> WideCha…
f0c0c9d 
…r conversions (#6170)

This PR adds a check for string max length in Utf8FromUtf16 and Utf16FromUtf8, similar to flutter/flutter#99729 / flutter/flutter#94608.
yutaaraki-toydium pushed a commit to yutaaraki-toydium/plugins that referenced this pull request Aug 12, 2022
@tgucio @yutaaraki-toydium
[camera_windows] Check string size before Win32 MultiByte <-> WideCha…
c109437 
…r conversions (flutter#6170)

This PR adds a check for string max length in Utf8FromUtf16 and Utf16FromUtf8, similar to flutter/flutter#99729 / flutter/flutter#94608.
moisefeelin pushed a commit to feelinproject/plugins that referenced this pull request Aug 26, 2022
@tgucio @moisefeelin
[camera_windows] Check string size before Win32 MultiByte <-> WideCha…
0c61fc5 
…r conversions (flutter#6170)

This PR adds a check for string max length in Utf8FromUtf16 and Utf16FromUtf8, similar to flutter/flutter#99729 / flutter/flutter#94608.
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
@tgucio @mauricioluz
[camera_windows] Check string size before Win32 MultiByte <-> WideCha…
aa43a36 
…r conversions (flutter#6170)

This PR adds a check for string max length in Utf8FromUtf16 and Utf16FromUtf8, similar to flutter/flutter#99729 / flutter/flutter#94608.
engine-flutter-autoroll pushed a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 22, 2023
@tgucio
[camera_windows] Check string size before Win32 MultiByte <-> WideCha…
6195f9e 
…r conversions (flutter#6170)

This PR adds a check for string max length in Utf8FromUtf16 and Utf16FromUtf8, similar to flutter/flutter#99729 / flutter/flutter#94608.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cbracken cbracken cbracken approved these changes

Assignees

No one assigned

Labels

a: desktop Running on desktop c: contributor-productivity Team-specific productivity, code health, technical debt. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tgucio @christopherfujino @cbracken