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

Compiling in VS 2015 as C++ #37

Closed
SparkDustJoe opened this issue Nov 16, 2015 · 4 comments
Closed

Compiling in VS 2015 as C++ #37

SparkDustJoe opened this issue Nov 16, 2015 · 4 comments

Comments

@SparkDustJoe
Copy link

Microsoft seems to have two specific beefs with the code as it exists now. Everything else compiles without warning or issue:

1 encoding.c, the definition for EQ(x, y), throws C4146 which prevents compilation
https://msdn.microsoft.com/query/dev14.query?appId=Dev14IDEF1&l=EN-US&k=k(C4146)&rd=true
In order to compile the code I suppress the warning each time it comes up, as I don't know a way around this.

#pragma warning(suppress: 4146)

2 encoding.c, the definition for SX(x) uses sprintf which Microsoft deems unsafe (and prevents compilation), so they recommend using sprintf_s which includes a buffer length (4 parameters instead of 3) to allow for safety checks inside the function itself. Easy fix.

#define SX(x) \ do { \ char tmp[30]; \ sprintf_s(tmp, 30, "%lu", (unsigned long)(x)); \ SS(tmp); \ } while (0)

sneves added a commit that referenced this issue Nov 16, 2015
@sneves
Copy link
Contributor

sneves commented Nov 16, 2015

I've addressed your first issue in a portable manner. The second one is trickier, since people do not seem to agree on what a bounded sprintf looks like. VS 2015 supports the standard C99 snprintf, but previous versions of VS do not, where _snprintf has different behavior (no null character guaranteed at the end).

Since there is no risk of overflow here, I would probably suggest using the _CRT_SECURE_NO_WARNINGS macro to get around this particular warning.

@SparkDustJoe
Copy link
Author

Does that macro have to be applied globally, or can it be applied per source file? I'm of the mind set that if only a small piece of code throws a security warning, you don't disable that warning for the whole project if it could potentially come up elsewhere in later code changes.

I would agree, in this context, the buffers in question are well contained in code that is not externally or user accessible so the risk is low.

@SparkDustJoe
Copy link
Author

I will also add, in encoding.c and run.c, there are warnings thrown when compiling as x64 instead of Win32, that storing a size_t in a uint32_t or unsigned int could cause possible loss of data.

These are in the definitions of CC_opt and BIN, and in run.c it's only for the line pwdlen = strlen(pwd); which I don't think is an issue in that last case, as most people are going to use passwords less than a few hundred characters (or a key file of less than 1kb).

@sneves
Copy link
Contributor

sneves commented Nov 16, 2015

Macro only needs to be applied locally; e.g., put #define _CRT_SECURE_NO_WARNINGS before any includes in src/encoding.c.

I've fixed the warning in src/run.c. The similar warnings in src/encoding.c are inside macros, and the code becomes confusing and difficult to reason about if I just cast to uint32_t. I'll think about a proper fix.

@technion technion mentioned this issue Nov 18, 2015
@sneves sneves closed this as completed in cf1883a Dec 4, 2015
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

No branches or pull requests

2 participants