Add compiler switches replace dangerous function with safer ones. #5089
Conversation
src/libpsl-native/CMakeLists.txt
Outdated
| # Can't use add_compile_options with 2.8.11 | ||
| set(CMAKE_BUILD_TYPE "Release") | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -Werror -fstack-protector-strong -fpie") | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -Werror -fstack-protector-strong -fpie -DFORTIFY_SOURCE=2") |
There was a problem hiding this comment.
Based on the documentation, FORTIFY_SOURCE=2 isn't enabled unless you also use -O2 optimizations
src/libpsl-native/CMakeLists.txt
Outdated
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -Werror -fstack-protector-strong -fpie -DFORTIFY_SOURCE=2 -O2") | ||
|
|
||
| if (${CMAKE_SYSTEM_NAME} MATCHES "Windows") | ||
| set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,-z,relro,-z,now") |
There was a problem hiding this comment.
Will CMAKE_SYSTEM_NAME ever be set to 'Windows'?
Can you please share some resources where I can find what each of those options is for?
There was a problem hiding this comment.
This component isn't built for Windows. The Windows flags should go in ...\src\powershell-native\coreclr_defs.cmake.
There was a problem hiding this comment.
This Windows section should be removed
There was a problem hiding this comment.
Removed the windows case.
The docs are here:
https://cmake.org/cmake/help/v3.4/variable/CMAKE_HOST_SYSTEM_NAME.html#variable:CMAKE_HOST_SYSTEM_NAME
and
https://cmake.org/cmake/help/v3.4/variable/CMAKE_SYSTEM_NAME.html
BTW, we use this when compiling for ARM, set(CMAKE_SYSTEM_NAME Linux).
There was a problem hiding this comment.
@TravisEz13 Thanks. I'm actually interested in the options like -Wl,-z,relro,-z,now, and would like to know what they mean. I quickly search them bug didn't find anything very useful.
It will be great if you can provide some pointers. And better, put the link in CMakeLists.txt as comments.
There was a problem hiding this comment.
What about the other aspect of my comment? Are there corresponding flags that we can or should use with MSBuild when compiling pwrshplugin.dll?
There was a problem hiding this comment.
I have another work item to run the scan that would detect these type of issues. I just merged a change to the build to give me the data to run the scan. I should have it done Monday.
|
@daxian-dbw Can you update your review? |
daxian-dbw
left a comment
daxian-dbw
left a comment
There was a problem hiding this comment.
It would be great if you add comments about some links that has information about the options used here, like -Wl,-z,relro,-z,now
|
I'll likely be making more changes. I'll include the detail where the tools give me them. |
4 participants
This should fix #4975