Apply audit mode to TerminalInput/Adapter/Parser libraries by miniksa · Pull Request #4005 · microsoft/terminal

Michael Niksa (miniksa) · 2019-12-17T23:14:18Z

Summary of the Pull Request

Enables auditing of Virtual Terminal libraries (input, adapter, parser)

PR Checklist

Rolls audit out to more things
I work here
Tests should still pass
Am core contributor
Closes Update MouseInput.cpp's _Generate*Sequence() to use wil::str_printf #3957

Detailed Description of the Pull Request / Additional comments

This is turning on the auditing of these projects (as enabled by the heavier lifting in the other refactor) and then cleaning up the remaining warnings.

Validation Steps Performed

Built it
Ran the tests

…structs, propagate changes out.

…eferences, string views, standard types.

…ng string views everywhere, size_ts over smaller sizes in prep for massive buffers, and so on and so on.

…ing used for calling ProcessString on the parser in the tests.

…sts. Fixed some rule of 5s, suppressed others. Suppressed constexpr suggestions. Use narrow_cast where applicable. Consts on unchanged variables. .at() for array accesses, and so on.

… constexpr and gluing everything back together. Otherwise, the usual suspects of array-to-pointer decay and noexcepts and whatnot.

…opriate.

Mike Griese (zadjii-msft)

Yea this one's good too. Okay, no more reviews from me today :P

Dustin L. Howett (MSFT) (DHowett-MSFT)

I'm NAKing out of this PR early to mark it red, but I'll keep reviewing.

Nit: please reword the PR title to be imperative, and as though it was completing the sentence "This pull request will ...".

…de (and in the future do conversions and whatnot safelyish). Use it to bypass the warnings in a methodic way for where we already know the bounds FOR SURE. Also, try to adjust some of adapt dispatch's arithmetic to make it not substantially worse and still pass audit before we get to improving our safemath library.

… a few other odds and ends.

…ach file. Run code format.

… pass. We can adjust the behavior re: clamping later.

Michael Niksa (miniksa)

(What happened here? a revert?)

Yeah, I thought the GitHub UI would allow me to revert one file in one commit. Turns out it made me revert a lot more than that.

…unity.

Michael Niksa (miniksa) · 2020-01-02T23:39:59Z

Gotta run to an appointment. If this succeeds everything, then we'll merge it at the next opportunity and move on to gardening3_audit2.

Deleted user (ghost) · 2020-01-03T14:25:09Z

Hello Mike Griese (@zadjii-msft)!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Michael Niksa (miniksa) added 14 commits December 12, 2019 10:56

Adjust the public methods to the state machine to use more modern con…

7461678

…structs, propagate changes out.

State Machine, Output State Machine Engine, and Tracing classes use r…

169bfb3

…eferences, string views, standard types.

InputStateMachine and associated fallout

64d1496

Dropping hungarians, using unique_ptr constructors, BOOL to bool, usi…

fee4b54

…ng string views everywhere, size_ts over smaller sizes in prep for massive buffers, and so on and so on.

Fix logical conversion errors discovered by running test suite.

dcf0e8f

Remove many but probably not all of the std::wstring intermediates be…

d97a4da

…ing used for calling ProcessString on the parser in the tests.

found the other one.

68bc729

Fix a few things I found in self-review.

e73d516

Merge branch 'master' into dev/miniksa/gardening3

3a92383

Merge branch 'master' into dev/miniksa/gardening3

a2c234a

code formatting run.

576da99

TerminalParser is now audit clean. Noexcepts propagated. Removed C-ca…

1616801

…sts. Fixed some rule of 5s, suppressed others. Suppressed constexpr suggestions. Use narrow_cast where applicable. Consts on unchanged variables. .at() for array accesses, and so on.

TerminalInput is now audited too. Most of it was moving the tables to…

e769ad3

… constexpr and gluing everything back together. Otherwise, the usual suspects of array-to-pointer decay and noexcepts and whatnot.

Enable audit on TerminalAdapter too.

608e10c

Michael Niksa (miniksa) self-assigned this Dec 17, 2019

Michael Niksa (miniksa) added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase labels Dec 17, 2019

Michael Niksa (miniksa) mentioned this pull request Dec 17, 2019

Make the terminal parser/adapter and related classes use modern, safe structures #3956

Merged

5 tasks done

Oops, setting noexcept had effects in the children. Propagate as appr…

f78f328

…opriate.

Mike Griese (zadjii-msft) approved these changes Dec 18, 2019

View reviewed changes

Comment thread src/terminal/adapter/MouseInput.hpp Resolved

Mike Griese (zadjii-msft) added the Needs-Second It's a PR that needs another sign-off label Dec 18, 2019

ghost requested review from Dustin L. Howett (MSFT) (DHowett-MSFT) and Carlos Zamora (carlos-zamora) December 18, 2019 17:02

Dustin L. Howett (MSFT) (DHowett-MSFT) reviewed Dec 18, 2019

View reviewed changes

Comment thread src/terminal/adapter/MouseInput.cpp Resolved

Dustin L. Howett (MSFT) (DHowett-MSFT) suggested changes Dec 18, 2019

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 18, 2019

Michael Niksa (miniksa) changed the title ~~Applies audit mode to TerminalInput/Adapter/Parser libraries~~ Apply audit mode to TerminalInput/Adapter/Parser libraries Dec 18, 2019

Dustin L. Howett (MSFT) (DHowett-MSFT) suggested changes Dec 18, 2019

View reviewed changes

James Holderness (j4james) reviewed Dec 26, 2019

View reviewed changes

Comment thread src/terminal/adapter/adaptDispatch.cpp Resolved

Michael Niksa (miniksa) changed the base branch from dev/miniksa/gardening3 to master December 31, 2019 18:39

Michael Niksa (miniksa) added 2 commits December 31, 2019 13:11

Merge branch 'master' into dev/miniksa/gardening3_audit

cfca853

Michael Niksa (miniksa) mentioned this pull request Jan 2, 2020

Convert THROW_IF_NULL_ALLOC for input parameters to return E_INVALIDARG not E_OUTOFMEMORY. #4099

Closed

Michael Niksa (miniksa) added 7 commits January 2, 2020 11:43

Comments from Dustin about references, function level try blocks, and…

fc49c35

… a few other odds and ends.

Drop constexpr suppressions and just make them static constexprs in e…

fc05775

…ach file. Run code format.

Revert adaptDispatch.cpp

d28c8c1

Restore overzealous revert.

8853d1a

Correct SA failures but still using safemath. Ensure unit tests still…

796aea3

… pass. We can adjust the behavior re: clamping later.

suppress warning on clearly incorrect c26496 detection.

5cc2943

fix the code format

fdd4dcc

Dustin L. Howett (MSFT) (DHowett-MSFT) approved these changes Jan 2, 2020

View reviewed changes

Comment thread src/terminal/adapter/adaptDispatch.cpp Resolved

Comment thread src/terminal/adapter/adaptDispatchGraphics.cpp Resolved

Comment thread src/terminal/parser/tracing.cpp Resolved

tinker with this some more to avoid x86 signed/unsigned mismatch.

8a82b2c

Michael Niksa (miniksa) commented Jan 2, 2020

View reviewed changes

a few more comments on til::at, safe math, and a missed rename opport…

a009ef1

…unity.

Dustin L. Howett (MSFT) (DHowett-MSFT) approved these changes Jan 2, 2020

View reviewed changes

Michael Niksa (miniksa) added 2 commits January 2, 2020 15:20

Merge branch 'master' into dev/miniksa/gardening3_audit

916a9de

Merge branch 'master' into dev/miniksa/gardening3_audit

e4115ee

Michael Niksa (miniksa) removed the Needs-Second It's a PR that needs another sign-off label Jan 2, 2020

Mike Griese (zadjii-msft) added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 3, 2020

ghost merged commit 322989d into master Jan 3, 2020

ghost deleted the dev/miniksa/gardening3_audit branch January 3, 2020 14:25

Mike Griese (zadjii-msft) mentioned this pull request Jan 3, 2020

Apply audit mode to TerminalConnection/Core/Settings and WinConPTY libraries #4016

Merged

6 tasks done

Michael Niksa (miniksa) restored the dev/miniksa/gardening3_audit branch January 3, 2020 15:23

Michael Niksa (miniksa) deleted the dev/miniksa/gardening3_audit branch January 3, 2020 15:24

James Holderness (j4james) mentioned this pull request Jan 3, 2020

Add support for all the line feed control sequences #3271

Merged

4 of 5 tasks

Dustin L. Howett (MSFT) (DHowett-MSFT) mentioned this pull request Jan 7, 2020

[REGR #3956] Terminal incorrectly splits part of escape sequence #4131

Closed

This pull request was closed.

Conversation

Michael Niksa (miniksa) commented Dec 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Dustin L. Howett (MSFT) (DHowett-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Michael Niksa (miniksa) left a comment

Choose a reason for hiding this comment

Uh oh!

Michael Niksa (miniksa) commented Jan 2, 2020

Uh oh!

Deleted user (ghost) commented Jan 3, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Michael Niksa (miniksa) commented Dec 17, 2019 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.