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

UI: Set invalid parameter handler in non-debug builds #10836

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

derrod
Copy link
Member

@derrod derrod commented Jun 12, 2024

Description

Sets the invalid parameter handler for Windows's "secure" functions like strcat_s(). By having one that does not abort execution we let the caller handle the error codes, instead of crashing to desktop.

Motivation and Context

We have error handling, we don't need Windows to abort execution of the program.

How Has This Been Tested?

Tested with issues found with #10834

Types of changes

  • Tweak (non-breaking change to improve existing functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@derrod derrod added the Windows Affects Windows label Jun 12, 2024
@derrod derrod requested a review from notr1ch June 12, 2024 21:15
@notr1ch
Copy link
Member

notr1ch commented Jun 13, 2024

This seems to go against Microsoft's recommendations:

When the framework calls the invalid parameter function, it usually means that a nonrecoverable error occurred. The invalid parameter function should save any data it can and then abort. It should not return control to the main function unless you are confident that the error is recoverable.

Specifically for strcat_s, it seems this would be called only when there is a programming error:

If strDestination is a null pointer, or isn't null-terminated, or if strSource is a NULL pointer, or if the destination string is too small, the invalid parameter handler is invoked, as described in Parameter validation.

In which case, I think I would prefer it invoke the crash handler. If we don't want this parameter validation we can use other non-throwing string functions such as StringCbCatA.

@derrod
Copy link
Member Author

derrod commented Jun 13, 2024

Specifically for strcat_s, it seems this would be called only when there is a programming error:

In this case there is one, see #10837, which results in a non-terminated string.

However, strcat_s will return EINVAL in this case, which could be gracefully handled. We generally treat invalid input into our string conversion functions as non-fatal if we can detect it and simply return a status that indicates the data is invalid. This behaviour of the Microsoft implemenation to call a separate function is different from the C11 standard, which merely requires a non-zero return code. It seems weird for this specific case to cause a crash when it is decidedly not unrecoverable.

In which case, I think I would prefer it invoke the crash handler. If we don't want this parameter validation we can use other non-throwing string functions such as StringCbCatA.

I haven't tried the crash handler yet, I wonder if it would give us anything useful in non-debug builds where all the descriptiors are NULL?

@RytoEX RytoEX added this to the OBS Studio 31 milestone Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Windows Affects Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants