Skip to content

Refactor regex code #2003

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

Closed
wants to merge 1 commit into from
Closed

Conversation

WGH-
Copy link
Contributor

@WGH- WGH- commented Jan 16, 2019

While proceeding with preparation of my RE2 patch (#1996), I found quite a few odd things with regex code, and decided to attempt to fix them first:

  • Lack of encapsulation.
  • Non-method functions for matching without retrieving all groups.
  • Regex class being copyable without proper copy-constructor (potential UAF
    and double free due to pointer members m_pc and m_pce).
  • Redundant SMatch::m_length, which always equals to match.size() anyway.
  • Weird SMatch::size_ member which is initialized only by one of the three matching
    functions, and equals to the return value of that function anyways.
  • Several places in code having std::string value instead of reference.

This commit fixes quite a few odd things in regex code:
 * Lack of encapsulation.
 * Non-method functions for matching without retrieving all groups.
 * Regex class being copyable without proper copy-constructor (potential UAF
   and double free due to pointer members m_pc and m_pce).
 * Redundant SMatch::m_length, which always equals to match.size() anyway.
 * Weird SMatch::size_ member which is initialized only by one of the three matching
   functions, and equals to the return value of that function anyways.
 * Several places in code having std::string value instead of reference.
@zimmerle zimmerle self-requested a review January 18, 2019 03:07
@zimmerle zimmerle added the 3.x Related to ModSecurity version 3.x label Jan 18, 2019
@zimmerle zimmerle self-assigned this Jan 18, 2019
@zimmerle
Copy link
Contributor

Hi @WGH-,

Thank you for the patch. Having a look at it tomorrow.

@zimmerle
Copy link
Contributor

Patch merged! Made minimal cosmetic changes to make the style check script happy again.

$ make check-coding-style | grep regex 
warning: ./src/utils/regex.cc:43:  { should almost always be at the end of the previous line  [whitespace/braces] [4]
warning: ./src/utils/regex.cc:112:  Closing ) should be moved to the previous line  [whitespace/parens] [2]

I do have suggestions for more refactoring surround this particular implementation. I will send it over you via patch.

@mdunc
Copy link

mdunc commented Jan 24, 2019

I think this caused my build to fail when compiling ModSecurity for nginx. Configured with the --with-lmdb option.

collection/backend/lmdb.cc: In member function ‘virtual void modsecurity::collection::backend::LMDB::resolveRegularExpression(const string&, std::vector<const modsecurity::VariableValue*>*, modsecurity::Variables::KeyExclusions&)’:
collection/backend/lmdb.cc:541:38: error: use of deleted function ‘modsecurity::Utils::Regex::Regex(const modsecurity::Utils::Regex&)’

Edit: Created an issue for this #2008

WGH- added a commit to WGH-/ModSecurity that referenced this pull request Jan 27, 2019
LMBD is not built by default since 6143eb9,
so add explicit --with-lmdb configuration.

Missing --with-lmdb build allowed a bug in PR owasp-modsecurity#2003 to pass
through, causing issue owasp-modsecurity#2008.
zimmerle pushed a commit that referenced this pull request Jan 28, 2019
LMBD is not built by default since 6143eb9,
so add explicit --with-lmdb configuration.

Missing --with-lmdb build allowed a bug in PR #2003 to pass
through, causing issue #2008.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants