Skip to content

mod_sed: review fixes, plus a test suite - #769

Closed
notroj wants to merge 8 commits into
apache:trunkfrom
notroj:mod-sed-fixes
Closed

notroj wants to merge 8 commits into
apache:trunkfrom
notroj:mod-sed-fixes

Conversation

@notroj

@notroj notroj commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

Commentary from Claude:

A review of mod_sed and the sed/regexp code it bundles, with a test/modules/filters suite to go with it.

The response filter change is the one worth a close look. On a failed evaluation it skipped both the final flush and the ap_pass_brigade(), taking the EOS with them, so the header filters never ran and the client was left on a response that never finished. It now separates a failure before anything has been written, where the caller can still turn it into an error response and does, from one after, where the response is already on the wire and has to be ended as the truncated one it has become.

The remaining fixes are independent of each other: array handling when filling eval->abuf, l deriving its octal escapes from a sign-extended char, eval_errf() taking a format string from a runtime value, and a starred backreference to a capture which matched nothing, where neither loop advances the input pointer.

test/pyhttpd/env.py gains add_cgi_module() so a suite can ask for CGI without naming mod_cgid, and the win32-only cgid→cgi substitution now applies everywhere, so a build without mod_cgid falls back rather than failing. docs/manual/mod/mod_sed.xml drops .htaccess from both directives' context lists, since they are ACCESS_CONF.

🤖 Generated with Claude Code

notroj and others added 8 commits September 23, 2026 19:56
* modules/filters/mod_sed.c (sed_filter_ctxt): Add passed, evalerr.
  (keep_metadata): New.
  (append_bucket, sed_response_filter): Track whether anything has
  reached the next filter.
  (sed_response_filter): On a failed evaluation, pass the metadata on
  and go on doing so for the rest of the response, unless nothing has
  been written yet; carry metadata buckets other than EOS and FLUSH
  through rather than deleting them; don't take the first bucket of an
  empty brigade.

* changes-entries/sed-eos-on-error.txt: Add.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
  NULL when filling eval->abuf, which otherwise runs one past the end
  of the array and over eval->aptr itself.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
  case writes off the unsigned byte value, not off a sign-extended
  char.

* changes-entries/sed-l-command.txt: Add.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
  attribute, and don't pass a runtime string as the format.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
  a capture which matched nothing as matching once; both loops step the
  input pointer by the capture length and so never terminate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
  so drop .htaccess from their context list.

[skip ci]

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
  HttpdTestSetup.add_cgi_module): New, so a suite can ask for CGI
  without naming mod_cgid.
  (HttpdTestSetup._make_modules_conf): Use _cgi_module() for a
  requested "cgid" on every platform, not just Windows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test/modules/filters/env.py (FiltersTestSetup.__init__): Load a CGI
  module.

* test/modules/filters/htdocs/test1/cgi/echo.py,
  test/modules/filters/htdocs/test1/cgi/nobody.py: New.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
asf-gitbox-commits pushed a commit that referenced this pull request Sep 24, 2026
* modules/filters/mod_sed.c (sed_filter_ctxt): Add passed, evalerr.
  (keep_metadata): New.
  (append_bucket, sed_response_filter): Track whether anything has
  reached the next filter.
  (sed_response_filter): On a failed evaluation, pass the metadata on
  and go on doing so for the rest of the response, unless nothing has
  been written yet; carry metadata buckets other than EOS and FLUSH
  through rather than deleting them; don't take the first bucket of an
  empty brigade.

* changes-entries/sed-eos-on-error.txt: Add.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub: PR #769


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1938485 13f79535-47bb-0310-9956-ffa450edef68
asf-gitbox-commits pushed a commit that referenced this pull request Sep 24, 2026
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub: PR #769


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1938486 13f79535-47bb-0310-9956-ffa450edef68
asf-gitbox-commits pushed a commit that referenced this pull request Sep 24, 2026
  case writes off the unsigned byte value, not off a sign-extended
  char.

* changes-entries/sed-l-command.txt: Add.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub: PR #769


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1938487 13f79535-47bb-0310-9956-ffa450edef68
asf-gitbox-commits pushed a commit that referenced this pull request Sep 24, 2026
  attribute, and don't pass a runtime string as the format.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub: PR #769


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1938488 13f79535-47bb-0310-9956-ffa450edef68
asf-gitbox-commits pushed a commit that referenced this pull request Sep 24, 2026
  a capture which matched nothing as matching once; both loops step the
  input pointer by the capture length and so never terminate.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub: PR #769


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1938489 13f79535-47bb-0310-9956-ffa450edef68
asf-gitbox-commits pushed a commit that referenced this pull request Sep 24, 2026
  so drop .htaccess from their context list.

[skip ci]

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub: PR #769


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1938490 13f79535-47bb-0310-9956-ffa450edef68
asf-gitbox-commits pushed a commit that referenced this pull request Sep 24, 2026
  HttpdTestSetup.add_cgi_module): New, so a suite can ask for CGI
  without naming mod_cgid.
  (HttpdTestSetup._make_modules_conf): Use _cgi_module() for a
  requested "cgid" on every platform, not just Windows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub: PR #769


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1938491 13f79535-47bb-0310-9956-ffa450edef68
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.

1 participant