We can also save stack space by changing:
char server_portstr[32];
to:
char server_portstr[6];
Or if we want to future-proof against the small possibility of a new TCP
standard that has larger port numbers and negative port numbers:
char server_portstr[sizeof(apr_port_t)*241/100+3]; /* log10(256) is
2.408 */
As part of my time caching work, I'm fixing a memory buffer issue in
mod_log_config.c. Here is the current code:
static const char *log_request_time_custom(request_rec *r, char *a,
apr_time_exp_t *xt)
{
apr_size_t retcode;
char tstr[MAX_STRING_LEN];
apr_strftime(tstr, &retcode, sizeof(tstr), a, xt);
return apr_pstrdup(r->pool, tstr);
}
This function needs the string on the heap, because the string is returned
to the caller. By first using stack memory, one has to add a memory copy
before returning. Also, this expands the stack by 8KiB, and stack might be
a limited resource on some OSes. The final problem is that the retcode
isn't checked: if the retcode is 0, the content of the buffer is undefined,
and shouldn't be used; in theory, even 8KiB might not be a big enough
buffer.
Here is my draft replacement:
static const char *log_request_time_custom(request_rec *r, char *a,
apr_time_exp_t *xt)
{
static const apr_size_t buf_len = 256;
apr_size_t tstr_len;
char *tstr = apr_palloc(r->pool, buf_len);
apr_strftime(tstr, &tstr_len, buf_len, a, xt);
return tstr_len ? tstr : "-";
}
On Thu, Dec 12, 2013 at 7:37 AM, Yann Ylavic <[email protected]> wrote:
> On Thu, Dec 12, 2013 at 1:54 AM, Kean Johnston <[email protected]>wrote:
>
>> I'd love to see these things fixed, because they add up. If you post them
>>> here they are likely to be reviewed very quickly, as they'll no doubt be
>>> simple to review.
>>>
>> Cool. Here's a patch for the case I just mentioned. It also eliminates an
>> un-needed automatic (yes, I obsess about stack frames too). diff against
>> trunk.
>>
>
> Note that in this particular cases (ie. "uri = apr_palloc(p,
> sizeof(*uri))" used in proxy_(ajp|fcgi|http|scgi|wstunnel)_handler), the
> "local" uri would better be declared on the stack (ie. apr_uri_t uri; and
> &uri used accordingly), so to avoid the allocation completely.
>
> Regards,
> Yann.
>
>
