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

Compression level validation #17

Merged
merged 4 commits into from
Oct 2, 2018

Conversation

bangbingsyb
Copy link
Contributor

@bangbingsyb bangbingsyb commented Sep 27, 2018

The PR is made to address: #15

iiszlib.dll has maximum allowable compression level 9, while the inbox gzip.dll has maximum level 10. This can not only cause confusion, but also zlib library directly crashes if the passed-in compression level is larger than 9. It would be extremely hard for users to figure out the root cause once the crash happens.

Main changes in the PR:

  1. Added compression level validation for both iiszlib and iisbrotli. If the compression level is out-of-bounds, IIS will simply return 500.19 to indicate a configuration error, and ask to check event log.
  2. Upon failure, both compression schemes will fire an event to application event log to indicate the current compression level VS the maximum value allowed. Each scheme will only fire one event per w3wp.exe lifetime to avoid spamming the event logs.

Other changes:

  • Updated the submodule reference to the latest IIS-Common
  • Updated .gitignore to neglect the .h, .rc files generated by mc.exe.

Future changes:

To let event viewer properly parse the message data, we need to set the following registries:

HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Eventlog\IIS Zlib:
EventMessageFile (REG_EXPAND_SZ): C:\Program Files\IIS\IIS Compression\iiszlib.dll
TypesSupported (REG_DWORD): 7

HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Services\Eventlog\IIS Brotli:
EventMessageFile (REG_EXPAND_SZ): C:\Program Files\IIS\IIS Compression\iisbrotli.dll
TypesSupported (REG_DWORD): 7

We need to update the installer of IIS Compression to write these registries.

@bangbingsyb
Copy link
Contributor Author

Configured iiszlib with compression level 12, and tested the event and the response:

image

image

@bangbingsyb bangbingsyb requested a review from a team September 27, 2018 18:01
@bangbingsyb bangbingsyb added the enhancement New feature or request label Sep 27, 2018
// so only the upper bound needs to be checked.
if (compression_level > BROTLI_MAX_QUALITY)
{
hr = ReportCompressionLevelOutOfBounds(compression_level, BROTLI_MAX_QUALITY);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need this hr assignment as you assign the value later

// so only the upper bound needs to be checked.
if (compression_level > BROTLI_MAX_QUALITY)
{
hr = ReportCompressionLevelOutOfBounds(compression_level, BROTLI_MAX_QUALITY);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the assignment

// so only the upper bound needs to be checked.
if (compression_level > Z_BEST_COMPRESSION)
{
hr = ReportCompressionLevelOutOfBounds(compression_level, Z_BEST_COMPRESSION);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the hr assignment

// so only the upper bound needs to be checked.
if (compression_level > Z_BEST_COMPRESSION)
{
hr = ReportCompressionLevelOutOfBounds(compression_level, Z_BEST_COMPRESSION);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the hr assigment

@bangbingsyb
Copy link
Contributor Author

@pan-wang Removed the redundant hr assignment.

@bangbingsyb bangbingsyb merged commit eb87fc9 into master Oct 2, 2018
@bangbingsyb bangbingsyb mentioned this pull request Oct 2, 2018
@bangbingsyb bangbingsyb deleted the bangbingsyb/compression-level-check branch October 2, 2018 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants