Skip to content

core: Add common apr_off_t config parser - #829

Open
arturobernalg wants to merge 1 commit into
apache:trunkfrom
arturobernalg:refactor/config-apr-offt-parser
Open

arturobernalg wants to merge 1 commit into
apache:trunkfrom
arturobernalg:refactor/config-apr-offt-parser

Conversation

@arturobernalg

Copy link
Copy Markdown
Member

Follow-up to the parsing discussion in #825.

Adds a common helper, ap_off_parameter_parse(), for converting configuration input to an apr_off_t, analogous to the existing timeout parser. It accepts a whole decimal number (as apr_strtoff() does) and fails on trailing characters, an empty value and overflow. It returns APR_EINVAL for a malformed number and APR_ERANGE for overflow, and only sets the value on success. The range of the value stays with each directive.

It converts the existing callers where the value is an apr_off_t or fits in one, and keeps each directive's own range checks (narrowing to long, int or apr_size_t is checked before the cast):

  • LimitRequestBody (no change in behavior or messages, checked by comparing the accepted values before and after)
  • CacheMinFileSize, CacheMaxFileSize, CacheReadSize, CacheReadTime (mod_cache_disk)
  • CacheSocacheMaxSize, CacheSocacheMaxTime, CacheSocacheMinTime, CacheSocacheReadSize, CacheSocacheReadTime
  • BufferSize, AuthFormSize
  • DavQuota
  • ScriptLogLength, ScriptLogBuffer (mod_cgi and mod_cgid)

This also makes malformed values such as 123abc fail at startup consistently instead of being partially accepted. Most of these directives used apr_strtoff() without checking the end of the number, and DavQuota, ScriptLogLength and ScriptLogBuffer used atol()/atoi(), so abc meant 0. Negative values of DavQuota (they also took the place of the Off/None values), ScriptLogLength and ScriptLogBuffer are now refused.

LimitXMLRequestBody is left to #825, and the directives with units (e.g. SubstituteMaxLineLength) are not touched.

The new function is exported API, so the MMN minor is bumped.

Tested with httpd -t against trunk and the patch for every directive above, and with new pyhttpd tests (core/cache/cgid, DavQuota, BufferSize). I also ran the pytest_suite cache, cgi, buffer, dav and limits tests, where nothing changed. The new tests were not run on Windows. mod_auth_form is only checked by that comparison, and the mod_cgi variant was only compiled.


  • This PR does not report or address a security vulnerability
    (see SECURITY.md and
    https://www.apache.org/security/#reporting-a-vulnerability).
  • Code follows the httpd style guide.
  • New log messages (level debug or higher) use an empty APLOGNO() tag;
    numbers are assigned by a committer at merge time (see docs/log-message-tags/README).
    (No new log messages.)
  • If the change is user-visible, a changes-entries/*.txt file is included,
    following the template in README.CHANGES (not needed otherwise).
  • Test cases based on pyhttpd are included where appropriate, in the
    test/modules/xxx directory matching the modules/xxx module source,
    or test/modules/core for core server changes. On Unix, verify these
    pass locally with e.g. make check-pytest PYTEST_DIRS=test/modules/xxx.

Add ap_off_parameter_parse(), a helper to parse a configuration value as an
apr_off_t, which fails on trailing characters and on overflow. The range of
the value remains up to the directive.

Use it for configuration directives which parsed such values in different
ways, some of them accepting trailing characters or no checks at all:
LimitRequestBody (unchanged behavior), ScriptLogLength, ScriptLogBuffer,
DavQuota, BufferSize, AuthFormSize, and the Cache*FileSize, Cache*ReadSize,
Cache*ReadTime and CacheSocache*Time directives.
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