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

Any plans to eliminate the many compile warnings, in e.g. Windows build? #1646

Closed
oviano opened this issue Nov 8, 2020 · 10 comments
Closed
Labels
[core] Area: Changes in SRT library core Epic Type: Maintenance Work required to maintain or clean up the code
Milestone

Comments

@oviano
Copy link
Contributor

oviano commented Nov 8, 2020

I use a custom VS2019 project with mostly default settings and the following warnings appear a lot:

C4101 - local variable never used
C4244 - conversion from type1 to type2 possible loss of data
C4267 - 'var' conversion from 'size_t' to 'type' possible loss of data

I also get the equivalent on other platforms, but maybe this is because I am not using the cmake system (I've not tried it)? I thought maybe the cmake system was downgrading some of these warnings but I couldn't find any reference to the above VS warnings in the repo, so assume not? Or maybe the overall warning level is lower or something?

Anyway, the trouble with lots of warnings is they hide possible non-trivial warnings, such as this one I get in a Debug x64 config when building core.cpp:

common.h (361): warning 4717: 'EventVariant::EventVariant<CPacket const*>': recursive on all control paths, function will cause runtime stack overflow.

@oviano oviano added the Type: Question Questions or things that require clarification label Nov 8, 2020
@maxsharabayko maxsharabayko added [core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code and removed Type: Question Questions or things that require clarification labels Nov 9, 2020
@oviano
Copy link
Contributor Author

oviano commented Nov 13, 2020

As an update to this, I addressed the warnings I was getting via this PR, which you can take if you want.

#1657

This disables the warnings via pragma. Addressing them would be better, and most of the could be addressed via casting but maybe that isn't the best solution and the types of some variables needs changing to be fixed properly. So in the meantime I did this.

@maxsharabayko
Copy link
Collaborator

Thank you, @oviano
Resolving warnings needs to be done.
We'll either take into work this issue directly or use PR #1657 as a permanent temporal solution.

@maxsharabayko maxsharabayko added this to the v1.4.3 milestone Nov 17, 2020
@maxsharabayko
Copy link
Collaborator

maxsharabayko commented Nov 23, 2020

TODO

Fix MSVC compiler warnings (316 when built without encryption):

@maxsharabayko
Copy link
Collaborator

Looks like a few warnings remain to be fixed.

\srtcore\common.h(1396,18): warning C4996: 'sscanf': This function or variable may be unsafe. Consider using sscanf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
\srtcore\logging.h(169,9): warning C4996: 'strcpy': This function or variable may be unsafe. Consider using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
\srtcore\logging.h(175,13): warning C4996: 'strcat': This function or variable may be unsafe. Consider using strcat_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
\srtcore\logging.h(176,13): warning C4996: 'strcat': This function or variable may be unsafe. Consider using strcat_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
\haicrypt\cryspr-openssl.c(51,51): warning C4267: 'function': conversion from 'size_t' to 'const int', possible loss of data
\haicrypt\cryspr-openssl.c(56,51): warning C4267: 'function': conversion from 'size_t' to 'const int', possible loss of data
\haicrypt\cryspr-openssl.c(166,54): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data
\haicrypt\cryspr-openssl.c(166,68): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data
\haicrypt\cryspr-openssl.c(166,80): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data

These are to be fixed by defaulting to OpenSSL EVP API (-DUSE_ENCLIB=openssl-evp).

\haicrypt\cryspr-openssl.c(51,13): warning C4996: 'AES_set_encrypt_key': Since OpenSSL 3.0
\haicrypt\cryspr-openssl.c(56,13): warning C4996: 'AES_set_decrypt_key': Since OpenSSL 3.0
\haicrypt\cryspr-openssl.c(127,75): warning C4996: 'AES_encrypt': Since OpenSSL 3.0

But those are to be fixed then:

\srt-max\haicrypt\cryspr-openssl-evp.c(52,48): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data
\srt-max\haicrypt\cryspr-openssl-evp.c(177,61): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data
\srt-max\haicrypt\cryspr-openssl-evp.c(146,45): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data
\srt-max\haicrypt\cryspr-openssl-evp.c(230,61): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data
\srt-max\haicrypt\cryspr-openssl-evp.c(344,45): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data
\srt-max\haicrypt\cryspr-openssl-evp.c(344,63): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data
\srt-max\haicrypt\cryspr-openssl-evp.c(344,78): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data

@oviano
Copy link
Contributor Author

oviano commented Feb 17, 2023

Unfortunately I build SRT by dropping the relevant source files directly into another project containing my own code, and I’m only set up to build using mbedTLS.

Happy to have a look based on the above logs though, when I am back from holiday next week.

@maxsharabayko
Copy link
Collaborator

@oviano Thank you for the heads up and for the contribution you've done already! The list of warnings is not that scary now 🙂

@oviano
Copy link
Contributor Author

oviano commented Feb 17, 2023

Now I just need to persuade you guys to turn on “warnings as errors” permanently 🙂

@maxsharabayko
Copy link
Collaborator

I don't need to be persuaded. As soon as it can be done - I'm in! 🙂

@oviano
Copy link
Contributor Author

oviano commented Mar 3, 2023

So I've addressed most of these here:

#2679

The only ones I've not addressed are the OpenSSL 3.0 deprecation ones as without building OpenSSL I can't really test any potential changes.

Also, let me know if you prefer a different solution to any of fixes for _CRT_SECURE_NO_WARNINGS.

@maxsharabayko
Copy link
Collaborator

@oviano

The only ones I've not addressed are the OpenSSL 3.0 deprecation ones as without building OpenSSL I can't really test any potential changes.

Those will go away if you build with -DUSE_ENCLIB=openssl-evp, which would likely become the default on in the future releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Epic Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

No branches or pull requests

2 participants