Skip to content
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.

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

Huge refactor of project files to make compilation security settings easier #3275

Draft
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

florelis
Copy link
Member

@florelis florelis commented May 26, 2023

Context

I have a bit of a story for this huge PR...

I am working on making sure we have all the right static analysis tools in the pipeline and resolve all warnings. Some warnings were related to using the right security settings for the compiler, like Spectre mitigations, or Control Flow Guard. It was bothering me how each time I added one to a project, VS was adding about eight lines like
<SDLCheck Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">true</SDLCheck>

So I thought "I'll manually edit the .vcproj files to remove the conditions so I can have just one instead of eight". Then I realized I was putting the same properties everywhere, and researched how to have common properties in a single file and then I got a bit carried away.

Changes

Added a Directory.Build.props file that is automatically included by MSBuild early in the process of reading the project files for all the files in the directory. This file includes all the common properties in all the C++ projects, but since it also would apply to the external projects and C# projects, it first detects what project it is in and does something different in each case.

  • For external projects, just enable the build security settings and disable all warnings
  • For C# projects, just set to treat warnings as errors
  • For C++ projects, set all the properties/items we had in all projects:
    • Set target platforms and toolset to use depending on VS version
    • Set properties like optimization, debug libraries, or preprocessor definitions depending on the configuration and platform
    • Set output directories
    • Set the security settings required by BinSkim (Spectre, ControlFlowGuard, SDLCheck, CETCompat and some others)
    • Enable code analysis in debug
    • Define all the macros we use to control the behavior in the pipelines
    • Set to use pch if the project has one
    • Set high warning level and treat them as errors

Removed everything that was duplicated in the project files. Also removed unused PropertySheet.props files. I'm pretty proud of this diff stat: 100 files changed, 502 insertions(+), 3261 deletions(-)

For static analysis:

  • Enabled CodeQL for the default branch
  • Updated the Guardian tasks (CredScan, BinSkim, publish logs) to the latest version as the old one is deprecated

Enabling code analysis and setting warnings at high level across the board created a lot of warnings/errors that I fixed:

  • For warnings in external dependencies (like WIL), I just disabled them there
  • Added a missing conversion from Win32 error to HResult
  • Added or modified type casts
  • Changed test code that compares HResult to include parenthesis, as there is something weird on Catch that triggers a warning about semantically different types when not passed like that
  • Added missing initialization to some member variables. I'm still not sure what setting enables that or why it only happens in some places
  • Refactored a test function that was using too much stack space

Notes

I'm opening this as draft because I haven't figured out some pipeline issues. The changes I did increase the build output size dramatically, from around 8GB to 11GB, which brings us over the free space available in the agent and causes build to fail.
I still have some code to remove. In the pipelines definition I have manually enabled CodeQL for my branch and added some build tasks to investigate the space issues, that needs to be removed. I also added a condition to the properties file to try and disable the security settings to see if that makes a difference, that also needs to be removed

Microsoft Reviewers: Open in CodeFlow

@Trenly
Copy link
Contributor

Trenly commented May 26, 2023

The changes I did increase the build output size dramatically, from around 8GB to 11GB

Would disabling RTTI (Run-Time Type Information) in some of the projects decrease the build size?

@yao-msft
Copy link
Contributor

Not sure how big of the impact when this is brought to internal build (Added a Directory.Build.props file that is automatically included by MSBuild). So I guess we would merge this after 1.5 release candidate.

@JohnMcPMS
Copy link
Member

The changes I did increase the build output size dramatically, from around 8GB to 11GB

Would disabling RTTI (Run-Time Type Information) in some of the projects decrease the build size?

It definitely is something to do to reduce final build size, but I have no idea how much it reduces intermediates size. We can probably do it, but it will take a bit of work to remove the dynamic_cast (implement our own RTTI-like handler just for the few cases) and of course validate that there was no UB introduced.

@JohnMcPMS
Copy link
Member

JohnMcPMS commented May 26, 2023

Not sure how big of the impact when this is brought to internal build (Added a Directory.Build.props file that is automatically included by MSBuild). So I guess we would merge this after 1.5 release candidate.

Also note that there is already such a file in the internal build and so we would need to be careful about how we get the properties since these projects need to build:

  1. In this repository, locally and in ADO
  2. In the internal repository, locally and in ADO, but inside a separate solution
  3. In the internal repository in ADO using this solution

Not sure what the lookup algorithm is for the props file, but I would guess that it isn't directly friendly to the tangled web we've woven. Also, the internal ADO builds use a different props file from the local builds...

I suppose what I'm saying is, maybe the props file that these projects use should be explicitly referenced as a project file relative path rather than using the implicit props file inclusion strategy of MSBuild.

@florelis
Copy link
Member Author

Not sure how big of the impact when this is brought to internal build (Added a Directory.Build.props file that is automatically included by MSBuild). So I guess we would merge this after 1.5 release candidate.

Oh, yeah. I was already thinking of that but forgot to write it in the description

Not sure what the lookup algorithm is for the props file, but I would guess that it isn't directly friendly to the tangled web we've woven. Also, the internal ADO builds use a different props file from the local builds...

The algorithm basically is "walk up until you find one", but it is possible to import one in upper levels to chain them. I'd need to figure out how to choose which one to use, though. Or change it to what you suggest.

I'm also planning on doing this same thing on the internal project, btw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants