[Bug 62223] New: httpd-2.4.33/modules/cache/mod_cache_socache.c:217]: limit check in wrong place ?

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62223

            Bug ID: 62223
           Summary: httpd-2.4.33/modules/cache/mod_cache_socache.c:217]:
                    limit check in wrong place ?
           Product: Apache httpd-2
           Version: 2.5-HEAD
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: mod_cache
          Assignee: bugs@httpd.apache.org
          Reporter: dcb314@hotmail.com
  Target Milestone: ---

Source code is

            while (apr_isspace(buffer[colon]) && (colon < *slider)) {

Suggest limit check before use, not after.

-- 
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@httpd.apache.org
For additional commands, e-mail: bugs-help@httpd.apache.org

[Bug 62223] httpd-2.4.33/modules/cache/mod_cache_socache.c:217]: limit check in wrong place ?

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62223

--- Comment #2 from Ruediger Pluem <rp...@apache.org> ---
(In reply to Yann Ylavic from comment #1)
> The goal is to not go above the '\r' here (which is the end of the buffer
> but still in the bounds).
> 
> So you raise a good optimization point since the last
> apr_isspace(buffer[colon]) check is always true/useless when colon ==
> *slider (and colon < *slider is probably a cheaper test), but it's not an
> out of bounds issue, right?

This is how I see it as well. Regarding the optimization:
I think in the usual case we do not hit the colon == *slider case. Hence we are
slightly better with the current code in the usual case where
apr_isspace(buffer[colon]) == 0 causes the while loop to end.

-- 
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@httpd.apache.org
For additional commands, e-mail: bugs-help@httpd.apache.org

[Bug 62223] httpd-2.4.33/modules/cache/mod_cache_socache.c:217]: limit check in wrong place ?

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62223

--- Comment #3 from David Binderman <dc...@hotmail.com> ---
(In reply to Yann Ylavic from comment #1)
> but it's not an out of bounds issue, right?

Could be. Is pr_isspace(buffer[ *slider]) an ok thing to do ?

In any event, it is standard operating procedure in C to sanity check
all array indexes before use. 

Whether or not it is faster, this is good style. Anyone reviewing
the code will be wondering why the sanity check is after, not before 
as usual.

-- 
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@httpd.apache.org
For additional commands, e-mail: bugs-help@httpd.apache.org

[Bug 62223] httpd-2.4.33/modules/cache/mod_cache_socache.c:217]: limit check in wrong place ?

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62223

Yann Ylavic <yl...@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |INVALID
             Status|NEW                         |RESOLVED

--- Comment #4 from Yann Ylavic <yl...@gmail.com> ---
(In reply to David Binderman from comment #3)
> 
> Whether or not it is faster, this is good style. Anyone reviewing
> the code will be wondering why the sanity check is after, not before 
> as usual.
Because it's only a stop condition, not a sanity check which is ensured by the
outer "while (*slider < buffer_len)" loop, hence buffer[*slider] is valid
access within the loop.

-- 
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@httpd.apache.org
For additional commands, e-mail: bugs-help@httpd.apache.org

[Bug 62223] httpd-2.4.33/modules/cache/mod_cache_socache.c:217]: limit check in wrong place ?

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=62223

--- Comment #1 from Yann Ylavic <yl...@gmail.com> ---
The goal is to not go above the '\r' here (which is the end of the buffer but
still in the bounds).

So you raise a good optimization point since the last
apr_isspace(buffer[colon]) check is always true/useless when colon == *slider
(and colon < *slider is probably a cheaper test), but it's not an out of bounds
issue, right?

-- 
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@httpd.apache.org
For additional commands, e-mail: bugs-help@httpd.apache.org