SDL signoff requirements - please enable additional C/C++ compiler warnings #66154
Comments
|
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
dotnet-issue-labeler
bot
added
the
untriaged
|
Added checkboxes... |
|
Tagging subscribers to this area: @dotnet/runtime-infrastructure Issue Details
Recent SDL changes require that we enable additional compiler warnings across our C/C++ projects. The requirements are:
Enable the following additional warnings, which must be fixed if they occur:
See the SDL guidelines (MSFT internal only) for further information. To make auditing code easier prior to signoff and release, please include the string Microsoft.Security.SystemsADM.10086 somewhere in the vcxproj / cmake / whatever file which is responsible for C/C++ compilation where all of these flags are defined. That gives the code auditors a target string to search against and validate that the proper warnings are enabled. See below an example of how to do this in a cmake file. # [[! Microsoft.Security.SystemsADM.10086 !]] - SQL required warnings
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/W3>) # warning level 3
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/WX>) # treat warnings as errors
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/we4018>) # 'expression' : signed/unsigned mismatch
add_compile_options($<$<COMPILE_LANGUAGE:C,CXX>:/we4055>) # 'conversion' : from data pointer 'type1' to function pointer 'type2'
# ... and so on ...Once the work is finished, or if there is no work to perform, please feel free to close this issue. Thanks for your assistance! Quick FAQWhat 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 forked 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. 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.
|
|
@GrabYourPitchforks Is this for the repo all up or only for coreclr and native code in libraries? |
This comment was marked as outdated.
This comment was marked as outdated.
Only for production code, so I'd think coreclr / mono (if compiled via MSVC++) / libs. Test code is excluded. Edit: From the same issue in the winforms repo, the rule of thumb I've been using to define "production" code is:
If it meets at least one of these bullets, we should consider it "production" code. If it's code that instead only ever executes as part of a test suite, it's non-production. |
danmoseley
added
the
help wanted
|
@AaronRobinsonMSFT in case anyone would like to help with this one, could you give a pointer to the necessary cmake changes? I am not familiar with cmake and I only found this, but it's for mono. runtime/src/mono/CMakeLists.txt Line 266 in 7db092a |
@danmoseley Sure. The way this issue is framed, it is confusing. I believe the entire repo already has runtime/eng/native/configurecompiler.cmake Lines 551 to 555 in b463b16 Note the removal of the default flag and then the conditional for CMake is added. If In #66154 (comment), I went through coreclr and found all the places we disabled one of the target warnings. In #66234, I am removing many of them for both mono and coreclr. However, even with the above PR some still remain. I've not checked the 4146 - Still exists in both mono and coreclr. Once #66234 is in, all others in the list have been handled in mono and coreclr. |
|
For mono, we need to pay attention to:
one complication - mono doesn't use eng/native/configurecompiler.cmake and sets its own compiler flags. So we need to verify all these warnings are on. |
Note that we have slowly adapted eng/native infra in mono; so it is semi-true these days. :)
Looking at @AaronRobinsonMSFT's fixes for coreclr, it should be easy to fix. I can take a stab at it (if nobody else is working on it). |
|
@am11 if you could handle 4267 in mono that would be great. I'll be working on 4146 and 4244 shortly. |
|
If anyone is willing to take a first scan of |
|
For the hosting components, a number of these are disabled - 4018, 4244, 4267, 4996: runtime/src/native/corehost/CMakeLists.txt Lines 13 to 17 in 853e494 runtime/src/native/corehost/apphost/static/CMakeLists.txt Lines 48 to 52 in 853e494 |
|
A few pragma suppressions in the host too: runtime/src/native/corehost/hostmisc/pal.h Line 161 in 853e494 runtime/src/native/corehost/hostmisc/pal.h Line 169 in 853e494 runtime/src/native/corehost/hostmisc/pal.h Line 174 in 853e494 |
|
Ugh. I forgot the src/native directory. |
|
@elinor-fung Can you provide a PR for the 4996 issues in |
|
Yep, I will do that. |
AaronRobinsonMSFT
removed
the
untriaged
|
@am11, @lambdageek, @elinor-fung Once #66427 is merged, I believe that |
|
A number of the additional warnings listed that must be fixed wouldn't be under
I know they are not enabled for the host. C4700 is level 1 and 4, but it is explicitly enabled by configurecompiler.cmake. |
|
@AaronRobinsonMSFT, what is the strategy around fixing the last two warnings levels on Mono? I just tried out 4244 and it issued around 2000 warnings, some in macros, but still a bunch to go through, guess that 4018 will generate a bunch as well. I guess we would need to fix most of them in different PR's and only enable them on the last one when most have already been fixed. Question is if/how this work should be distributed and what strategy we should use to handle it. |
@lateralusX Yes, there is going to be substantial changes to mitigate these. I tried going down this path but got a bit overwhelmed and took a step back to work on other tasks. I am not sure of the preferred strategy for Mono to mitigate these to be honest. There are so many and as someone who primarily works in CoreCLR I'm likely not the correct person to make a call on how to proceed. I am more than willing to help in any way that I can but I can't make the call myself and would prefer to hear from you, @lambdageek, or @vargaz on how you would like to mitigate them. |
OK, I can coordinate within Mono team, just wanted to make sure we don't end up doing similar work on different ends. I guess we could coordinate using this issue on what we plan to work on making sure (maybe that's wishful thinking on my end) no one else work on the same things. |
|
@lateralusX That is a fine solution. I agree using this task is feasible. I, along with @elinor-fung and likely @am11, would be willing to help with tasks if they could be clearly defined - perhaps down to compilation unit? |
I can probably work through component libraries first to get a feeling as well as the total amount of outstanding warnings after that work. Once that is done, we could divide on compile unit or sub area, like utils/metadata/mini (different folders), but they will probably need to come in dependency order, since fixing in one might also fix in others (due to shared headers). |
|
@lateralusX Have you had a chance to discuss with the rest of the mono team on this one? No pressure, just want to avoid a mad rush on this kind of issue prior to RC. |
|
I stand corrected. It looks like 4244 is still present under |
Finishing of another work item and then I will do a triage on the remaining ones and add them to this issue (probably fix a set of them during the triage), we could then distribute it withing the teams if needed. Hopefully I will have something done beginning of next week. |
|
Using this comment to track progress against fixing 4018 and 4244 on Mono.
Enabling 4018 on Mono Windows build generate
Enabling 4244 on Mono runtime Windows x86/x64 build generate
//CC @lambdageek @vargaz |
aligning with SDL requirements (dotnet#66154). There will be additional warnings in the cross compilers that needs to be fixed separately, but this should fix the major bulk of 4244 warnings. All changes done in this PR are mitigating compiler issued data type truncation 4244 warnings: 'conversion' conversion from 'type1' to 'type2', possible loss of data and PR will align to the same type as the compiler issued in the warning. PR introduce a new set of macros to cast between types following patter of existing GPOINTER_TO_INT, GPOINTER_TO_UINT, GTYPE1_TO_TYPE2. The idea of use these macros is that it will be clear where casts are done, and we will have ability to intercept and add validation logic in specialized builds to catch truncation errors. The PR also introduce the needed inline functions under ENABLE_CHECKED_CASTS that could be used for extended validation when using the macros. Failure actions in these inline functions are not currently fully implemented, since that will be different from case to case, but can be added when needed since all infrastructure is prepared. If it is possible to change types used in code to mitigate the warnings, that will be the initial strategy used by this PR, if that however would cause ripple effects through the source code (unfortunately quite common), we will fallback using the GTYPE1_TO_TYPE2 macros to clearly show the intent, keep trackability of the change as well as adding ability to do additional validation of casts in the future.
* Fixes all 4244 warnings on Mono runtime x86/x64 Windows build, aligning with SDL requirements (#66154). There will be additional warnings in the cross compilers that needs to be fixed separately, but this should fix the major bulk of 4244 warnings. All changes done in this PR are mitigating compiler issued data type truncation 4244 warnings: 'conversion' conversion from 'type1' to 'type2', possible loss of data and PR will align to the same type as the compiler issued in the warning. PR introduce a new set of macros to cast between types following patter of existing GPOINTER_TO_INT, GPOINTER_TO_UINT, GTYPE1_TO_TYPE2. The idea of use these macros is that it will be clear where casts are done, and we will have ability to intercept and add validation logic in specialized builds to catch truncation errors. The PR also introduce the needed inline functions under ENABLE_CHECKED_CASTS that could be used for extended validation when using the macros. Failure actions in these inline functions are not currently fully implemented, since that will be different from case to case, but can be added when needed since all infrastructure is prepared. If it is possible to change types used in code to mitigate the warnings, that will be the initial strategy used by this PR, if that however would cause ripple effects through the source code (unfortunately quite common), we will fallback using the GTYPE1_TO_TYPE2 macros to clearly show the intent, keep trackability of the change as well as adding ability to do additional validation of casts in the future.
|
@AaronRobinsonMSFT is CoreCLR work here now all done? |
|
@danmoseley The final piece of work should be merged no later than tmr. (#71269) |
|
W00t! Thank you @fanyang-mono @lambdageek @lateralusX @elinor-fung @am11 and anyone else who helped make this happen. @GrabYourPitchforks Do we have an audit plan from SDL. Just to make sure we didn't miss anything. |
|
@GrabYourPitchforks ? what remains to close this.. |
|
I think we're good here! Will reopen it if new things crop up. Thanks again for the efforts. :) |
GrabYourPitchforks
removed
the
help wanted
ghost
locked as resolved and limited conversation to collaborators
Recent SDL changes require that we enable additional compiler warnings across our C/C++ projects. The requirements are:
Enable the following additional warnings, which must be fixed if they occur:
See the SDL guidelines (MSFT internal only) for further information.
To make auditing code easier prior to signoff and release, please include the string Microsoft.Security.SystemsADM.10086 somewhere in the vcxproj / cmake / whatever file which is responsible for C/C++ compilation where all of these flags are defined. That gives the code auditors a target string to search against and validate that the proper warnings are enabled. See below an example of how to do this in a cmake file.
Once the work is finished, or if there is no work to perform, please feel free to close this issue.
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 forked 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.
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.
The text was updated successfully, but these errors were encountered: