Skip to content

Update to seclang-scanner changes introduced by Windows support #3146

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

Merged

Conversation

eduar-hte
Copy link
Contributor

@eduar-hte eduar-hte commented May 19, 2024

While testing the builds with parser generation in PR #3144 I found out that the changes introduced to seclang-scanner.cc in commit a488568 would likely be overwritten next time an update to the parser is introduced.

This cannot be avoided by introducing the required change (an #ifdef not to include unistd.h in the Windows build as it's not available in the MSVC C++ compiler, and include io.h instead) in seclang-scanner.ll because the code is always injected by Flex itself:

#ifndef YY_NO_UNISTD_H
/* Special case for "unistd.h", since it is non-ANSI. We include it way
 * down here because we want the user's section 1 to have been scanned first.
 * The user has a chance to override it with an option.
 */
/* %if-c-only */
#include <unistd.h>
/* %endif */
/* %if-c++-only */
/* %endif */
#endif

As suggested by the comment, this PR introduces the option to disable including unistd.h, as it's not required by the parser usage in libModSecurity. The options added to seclang-scanner.cc are: nounistd never-interactive (the first requires the second to be also set for the generated code to compile).

This PR also includes two minor additional changes related to Windows support.

eduar-hte added 3 commits May 19, 2024 16:38
- The parser is not used interactively so we can avoid including
  unistd.h, which is not available on Windows MSVC C++ compiler.
- The #ifdef WIN32 introduced in PR owasp-modsecurity#3132 would probably be overwritten
  when the parser is updated.
@eduar-hte
Copy link
Contributor Author

eduar-hte commented May 20, 2024

As suggested by the comment, this PR introduces the option to disable including unistd.h, as it's not required by the parser usage in libModSecurity. The options added to seclang-scanner.cc are: nounistd never-interactive (the first requires the second to be also set for the generated code to compile).

For the sake of reference:

  • An interactive scanner is one that only looks ahead to decide what token has been matched if it absolutely must. It turns out that always looking one extra character ahead, even if the scanner has already seen enough text to disambiguate the current token, is a bit faster than only looking ahead when necessary. But scanners that always look ahead give dreadful interactive performance; for example, when a user types a newline, it is not recognized as a newline token until they enter another token, which often means typing in another whole line.
  • Source: Flex manual - Options Affecting Scanner Behavior

In order to determine whether the scanner needs to be interactive, it calls the function isatty which is declared in unistd.h.

@airween
Copy link
Member

airween commented May 22, 2024

While testing the builds with parser generation in PR #3144 I found out that the changes introduced to seclang-scanner.cc in commit a488568 would likely be overwritten next time an update to the parser is introduced.

Oh gosh, I forgot to mention that - because I realized... sorry.

Yes, seclang-parser.cc and seclang-scanner.cc files are generated. Those depend not just on the content (tokens and grammar) but the Bison's version too.

So you should forget to touch them, because every modifications will be overwritten next time when someone adds a new configure directive or any language component.

This cannot be avoided by introducing the required change (an #ifdef not to include unistd.h in the Windows build as it's not available in the MSVC C++ compiler, and include io.h instead) in seclang-scanner.ll because the code is always injected by Flex itself:

#ifndef YY_NO_UNISTD_H
/* Special case for "unistd.h", since it is non-ANSI. We include it way
 * down here because we want the user's section 1 to have been scanned first.
 * The user has a chance to override it with an option.
 */
/* %if-c-only */
#include <unistd.h>
/* %endif */
/* %if-c++-only */
/* %endif */
#endif

Yes, this is how Flex work - but this is the expected behavior on every other systems, not just on Windows.

As suggested by the comment, this PR introduces the option to disable including unistd.h, as it's not required by the parser usage in libModSecurity. The options added to seclang-scanner.cc are: nounistd never-interactive (the first requires the second to be also set for the generated code to compile).

Please remove the seclang-scanner.cc modifications, those will be overwritten next time when someone modifies seclang-scanner.ll file.

This PR also includes two minor additional changes related to Windows support.

Right, thanks.

@eduar-hte
Copy link
Contributor Author

eduar-hte commented May 22, 2024

Yes, seclang-parser.cc and seclang-scanner.cc files are generated. Those depend not just on the content (tokens and grammar) but the Bison's version too.

Yes, I saw seclang-parser.yy while working on PR #3132 because at some point I thought I may need to introduce an update there and generate the .cc files again, but it was not necessary in the end. However, I didn't see seclang-scanner.ll when I introduced the WIN32 guard to prevent including unistd.h.

Please remove the seclang-scanner.cc modifications, those will be overwritten next time when someone modifies seclang-scanner.ll file.

That's ok, the version I included in this PR is the one generated with the updated version of seclang-scaner.ll which removes the manual changes mentioned before (and now includes the define that makes the parser non-interactive). The latest version of seclang-scanner.cc after updating seclang-scanner.ll needs to be committed for the builds that do not generate the parser (which is off by default).

- build/win32/* files from Windows builds, other files from Unix builds
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@airween
Copy link
Member

airween commented May 23, 2024

Yes, I saw seclang-parser.yy while working on PR #3132 because at some point I thought I may need to introduce an update there and generate the .cc files again, but it was not necessary in the end. However, I didn't see seclang-scanner.ll when I introduced the WIN32 guard to prevent including unistd.h.

okay,

That's ok, the version I included in this PR is the one generated with the updated version of seclang-scaner.ll which removes the manual changes mentioned before (and now includes the define that makes the parser non-interactive). The latest version of seclang-scanner.cc after updating seclang-scanner.ll needs to be committed for the builds that do not generate the parser (which is off by default).

Now I see, thank you. Approved, merging now.

Many thanks for your contribution.

@airween airween merged commit 2fd45f8 into owasp-modsecurity:v3/master May 23, 2024
45 of 46 checks passed
@eduar-hte eduar-hte deleted the seclang-scanner-nounistd branch May 23, 2024 13:51
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