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

SDL signoff requirements - enable MSVC warnings C4242 and C4244 (libunwind) #99471

Closed
GrabYourPitchforks opened this issue Mar 9, 2024 · 4 comments · Fixed by #100241
Closed

Comments

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Mar 9, 2024

Please prioritize this work. We want to address BinSkim alerts early in the development cycle.

Per the SDL guidelines (MSFT internal only), C/C++ warning C4242 and C4244 are required to be enabled, and fixing violations of it is mandatory.

In src/native/external/libunwind_extras/CMakeLists.txt, we also need to delete these two lines:

add_compile_options(-wd4242) # possible loss of data
add_compile_options(-wd4244) # possible loss of data

We will need to make changes to our local copy of libunwind as part of this. See the "What about third-party code?" section below.

Thanks for your assistance!

Quick FAQ

What code is bound to this requirement?

This affects only production code. Production code is generally defined as code which ships as part of the product and which runs on customer machines or which manages infrastructure, such as our build labs. Unit and functional test projects are not considered production code.

Does this need to be backported?

No backporting plans at this time. If actual bugs are found during this process, individual product teams have discretion to selectively backport into the next downlevel servicing vehicle.

What about third-party code?

This requirement applies to all code that MSFT builds from source, regardless of its provenance. Ideally any changes that we make to local forked copies can be submitted upstream as a PR so that the wider ecosystem can enjoy their benefits.

The recommended pattern - and what we did with zlib a while back - is to create a .patch file which contains our local fixes to libunwind, then commit this .patch file as part of the same PR where we make the changes. See #91245 for an example of how this was done.

If this is impractical, exceptions to this requirement can be sought on an as-needed basis. However, exceptions are: (a) not guaranteed to be granted; and (b) time-constrained. The exception process is not intended to provide a permanent deferral of this work. Please contact the fxsecurity alias if an exception is needed.

What about C# and other languages?

This requirement only affects C/C++ code. Requirements for other languages will be filed as separate issues.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Mar 9, 2024
@jkotas
Copy link
Member

jkotas commented Mar 9, 2024

Related to #90359

@jkotas jkotas changed the title SDL signoff requirements - enable MSVC warning C4242 SDL signoff requirements - enable MSVC warning C4242 (libunwind) Mar 9, 2024
@GrabYourPitchforks GrabYourPitchforks changed the title SDL signoff requirements - enable MSVC warning C4242 (libunwind) SDL signoff requirements - enable MSVC warnings C4242 and C4244 (libunwind) Mar 11, 2024
@tommcdon
Copy link
Member

@AaronRobinsonMSFT @janvorli

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Mar 12, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 9.0.0 milestone Mar 12, 2024
@filipnavara
Copy link
Member

Here's a commit with most of the work done: 71ab525

Feel free to pick it up.

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

Successfully merging a pull request may close this issue.

5 participants