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

Prevent uninitialized value bugs and improve string safety. #674

Merged
merged 1 commit into from
Aug 25, 2019

Conversation

beevik
Copy link
Contributor

@beevik beevik commented Aug 9, 2019

This change does two things:

1. Updates the registry APIs to reduce the likelihood of uninitialized
variables.

The code wasn't always checking the return value of registry load operations.
In some cases, this led to uninitialized memory being used, and crashes could
result. For example, LoadConfiguration in Applewin.cpp was using an
uninitialized value for the computer type if no registry variable for the
"Apple 2 type" was set.

New registry reading methods and macros have also been introduced, allowing
default value fallbacks for the cases where a registry variable is not found.
This makes registry access simpler and safer when a default value is known in
advance.

The registry code's style has also been updated to conform with the rest of
the code base (tabs instead of spaces, naming conventions, etc.)

2. Introduces string safety improvements.

A number of code paths have been modified to use safe-string functions instead
of their unsafe counterparts (e.g., strcpy, sprintf). In the process, some
strings were converted from "char" to "TCHAR". This was done mostly for
consistency with the rest of the code-base.

This change does two things:

1. Updates the registry APIs to reduce the likelihood of uninitialized
variables.

The code wasn't always checking the return value of registry load operations.
In some cases, this led to uninitialized memory being used, and crashes could
result. For example, LoadConfiguration in Applewin.cpp was using an
uninitialized value for the computer type if no registry variable for the
"Apple 2 type" was set.

New registry reading methods and macros have also been introduced, allowing
default value fallbacks for the cases where a registry variable is not found.
This makes registry access simpler and safer when a default value is known in
advance.

The registry code's style has also been updated to conform with the rest of
the code base (tabs instead of spaces, naming conventions, etc.)

2. Introduces string safety improvements.

A number of code paths have been modified to use safe-string functions instead
of their unsafe counterparts (e.g., strcpy, sprintf).  In the process, some
strings were converted from "char" to "TCHAR". This was done mostly for
consistency with the rest of the code-base.
@tomcw
Copy link
Contributor

tomcw commented Aug 10, 2019

Some quick feedback:

  • in general both of these two things are good improvements
  • we should try to get this reviewed and committed soon before it becomes stale (ie. as the mainline moves forwards)
  • I will be rejecting the switch from new/delete to malloc/free - so these changes need to be reverted
    • There's also 1 change from VirtualAlloc() to malloc(). Just use new.
  • Minor point: (my) newer code tends not be use Hungarian notation so much (there was a case where you added a "dw" prefix). Consistency with the original function should take precedence, so in this specific case (halfScanLines -> dwHalfScanLines in video.cpp) it's OK.

Thanks.

@beevik
Copy link
Contributor Author

beevik commented Aug 12, 2019

Oops, that second commit (the one that included malloc/free) wasn't meant for this pull request. I'll remove it.

tomcw added a commit that referenced this pull request Aug 25, 2019
. Prevent uninitialized value bugs and improve string safety. (PR #674)
@tomcw tomcw merged commit 9e5e21b into AppleWin:master Aug 25, 2019
@tomcw
Copy link
Contributor

tomcw commented Aug 25, 2019

For VS2008, I was getting lots of warning 4995's when compiling:
'function': name was marked as #pragma deprecated

...due to this addition to stdafx.h:
#include <strsafe.h>

So for VS2008 only (Debug & Release) I just included this in the Disable Specific Warnings.

@beevik
Copy link
Contributor Author

beevik commented Aug 27, 2019

Out of curiosity, what is the reason for continuing to support VS2008?

@tomcw
Copy link
Contributor

tomcw commented Aug 29, 2019

See this comment: #559 (comment)

Basically there are AppleWin users on very old versions of Windows, so I tend to stick with old versions of VS (from the History.txt, I switched to VS2008 in 2016).

I'd be good to move to a more current VS version, but of all the things to do, it doesn't feel particularly pressing.

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

Successfully merging this pull request may close these issues.

2 participants