Re: Do pools lead to bad programming?
Le 12/12/2013 01:15, Graham Leggett a écrit : Obviously allocating too early and then throwing away the results of the allocation is a waste as you've pointed out, and should ideally be smoked out and fixed. 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. Regards, Graham -- +1 as well I've gone quickly thrue code source and this kind of construction does not seem to be so common. Best regards, CJ
Re: Do pools lead to bad programming?
Le 12/12/2013 01:54, Kean Johnston a écrit : 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. Kean If interested in stack frame, you can have a look at http://mail-archives.apache.org/mod_mbox/httpd-dev/201306.mbox/%3Ckp1fo8$v6b$1...@ger.gmane.org%3E I've updated the files yesterday with latest trunk. If you wish, I can send the script used to generate them. Best regards, CJ
Re: Do pools lead to bad programming?
The format string is a multiplier of length. 4k repeated %Y elements in
the 'a' variable's format string is 8kbytes in the format string, but the
result would take 16kbytes. Then you have things like %B: the full month
name according to the current locale. You cannot really predict the length
without actually doing the strftime.
I figured 256 bytes is more than reasonable. Normal time strings are < 32
bytes; double that, and quadruple that again (e.g. if there are utf-8 chars
in the string), and you get 256 bytes. If people need more, one can use in
LogFormat %{formatstring1}t%{formatstring2}t.
On Fri, Dec 13, 2013 at 10:14 AM, Yann Ylavic wrote:
> On Fri, Dec 13, 2013 at 5:06 AM, Daniel Lescohier <
> daniel.lescoh...@cbsi.com> wrote:
>
>> 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 : "-";
>> }
>>
>
> Shouldn't [MAX_STRING_LEN > buf_len > strlen(a)] for apr_strftime() not to
> truncate the result (for any resonnable 'a')?
>
>
Re: Do pools lead to bad programming?
On Fri, Dec 13, 2013 at 5:06 AM, Daniel Lescohier wrote:
> 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 : "-";
> }
>
Shouldn't [MAX_STRING_LEN > buf_len > strlen(a)] for apr_strftime() not
to truncate the result (for any resonnable 'a')?
Re: Do pools lead to bad programming?
On Fri, Dec 13, 2013 at 5:06 AM, Daniel Lescohier wrote: > char server_portstr[sizeof(apr_port_t)*241/100+3]; /* log10(256) is > 2.408 */ > Nice :)
Re: Do pools lead to bad programming?
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 wrote:
> On Thu, Dec 12, 2013 at 1:54 AM, Kean Johnston 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.
>
>
Re: Do pools lead to bad programming?
On Thu, Dec 12, 2013 at 1:54 AM, Kean Johnston 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.
Re: Do pools lead to bad programming?
On Dec 11, 2013, at 7:15 PM, Graham Leggett wrote: > > Obviously allocating too early and then throwing away the results of the > allocation is a waste as you've pointed out, and should ideally be smoked out > and fixed. > Agreed.
Re: Do pools lead to bad programming?
On 12 Dec 2013, at 00:00, Kean Johnston wrote: > Hi all, > > So I've been spending a fair bit of time inside Apache recently and I've seen > a pattern. Consider the following code (from mod_proxy_fcgi.c): That's just minor sloppiness. Fixing it is a minor improvement, and if you have the time and inclination, go right ahead! > Am I being too obsessive? Nope. If you want to submit patches (bugzilla is probably better than this list) they have the advantage of being easy to review, so with luck they won't just languish there! Not sure I'm keen on your thread title. If pools lead to sloppiness, what about garbage collection? Let alone scripting languages! -- Nick Kew
Re: Do pools lead to bad programming?
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. Kean Index: modules/proxy/mod_proxy_fcgi.c === --- modules/proxy/mod_proxy_fcgi.c (revision 1550327) +++ modules/proxy/mod_proxy_fcgi.c (working copy) @@ -757,14 +757,11 @@ char server_portstr[32]; conn_rec *origin = NULL; proxy_conn_rec *backend; +apr_uri_t *uri; proxy_dir_conf *dconf = ap_get_module_config(r->per_dir_config, &proxy_module); -apr_pool_t *p = r->pool; - -apr_uri_t *uri = apr_palloc(r->pool, sizeof(*uri)); - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01076) "url: %s proxyname: %s proxyport: %d", url, proxyname, proxyport); @@ -790,7 +787,8 @@ backend->is_ssl = 0; /* Step One: Determine Who To Connect To */ -status = ap_proxy_determine_connection(p, r, conf, worker, backend, +uri = apr_palloc(r->pool, sizeof(*uri)); +status = ap_proxy_determine_connection(r->pool, r, conf, worker, backend, uri, &url, proxyname, proxyport, server_portstr, sizeof(server_portstr)); @@ -814,7 +812,7 @@ } /* Step Three: Process the Request */ -status = fcgi_do_request(p, r, backend, origin, dconf, uri, url, +status = fcgi_do_request(r->pool, r, backend, origin, dconf, uri, url, server_portstr); cleanup:
Re: Do pools lead to bad programming?
On 12/11/13 4:00 PM, Kean Johnston wrote: > Am I being too obsessive? If not, would you like patches to correct these as I > find them, and if so, should I open a bug about this or just post patches here > (they are all likely to be a simple move of 1 or 2 lines)? There are two ways this sort of thing can happen. The person who wrote the original code didn't feel like breaking the declaration of the variable and the allocation into two lines. Which is a mistake. Alternatively, the allocation could have always been necessary and the only difference was the ordering when it happened. I.E. there may have not been any returns before the use of the variable. This would be an error on the part of the person changing the code. Looking at this specific case, the variable was added in r357444 as part of the original template of mod_proxy_fcgi. So it seems like an error on the original developers part. It certainly seems worthwhile to clean these up when they are found in my opinion. I wouldn't say that pools lead to bad programming. It's mostly that pools limit the consequences of these sorts of mistakes. The mistakes are going to happen regardless.
Re: Do pools lead to bad programming?
On 12 Dec 2013, at 2:00 AM, Kean Johnston wrote:
> So I've been spending a fair bit of time inside Apache recently and I've seen
> a pattern. Consider the following code (from mod_proxy_fcgi.c):
>
>apr_uri_t *uri = apr_palloc(r->pool, sizeof(*uri));
>
>ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01076)
> "url: %s proxyname: %s proxyport: %d",
> url, proxyname, proxyport);
>
>if (strncasecmp(url, "fcgi:", 5) != 0) {
>ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01077) "declining
> URL %s", url);
>return DECLINED;
>}
>
> That allocation of uri can be moved down (further than the code shown above)
> until right before it is used. I've seen this in a number of places and it
> "feels" like it is considered OK because the pool is relatively short lived
> and in most cases I've seen so far the allocation is pretty small. But as a
> concept, this strikes me as bad. If that code was using a traditional
> malloc/free the above would be a memory leak. I am aware that pools provide
> protection against such leaks, but nonetheless, that is wasted memory
> allocated and although quick, also a waste of time.
The idea behind pools is that you allocate an arbitrary and unpredictable set
of memory, and then free the whole set at some future point in time. This means
you don't need to keep track of each and every escape path out of a system and
hope you've cleaned up everything, you allocate at will confident that it will
all be freed.
Obviously allocating too early and then throwing away the results of the
allocation is a waste as you've pointed out, and should ideally be smoked out
and fixed.
> Am I being too obsessive? If not, would you like patches to correct these as
> I find them, and if so, should I open a bug about this or just post patches
> here (they are all likely to be a simple move of 1 or 2 lines)?
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.
Regards,
Graham
--
Do pools lead to bad programming?
Hi all,
So I've been spending a fair bit of time inside Apache recently and I've
seen a pattern. Consider the following code (from mod_proxy_fcgi.c):
apr_uri_t *uri = apr_palloc(r->pool, sizeof(*uri));
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01076)
"url: %s proxyname: %s proxyport: %d",
url, proxyname, proxyport);
if (strncasecmp(url, "fcgi:", 5) != 0) {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01077)
"declining URL %s", url);
return DECLINED;
}
That allocation of uri can be moved down (further than the code shown
above) until right before it is used. I've seen this in a number of places
and it "feels" like it is considered OK because the pool is relatively
short lived and in most cases I've seen so far the allocation is pretty
small. But as a concept, this strikes me as bad. If that code was using a
traditional malloc/free the above would be a memory leak. I am aware that
pools provide protection against such leaks, but nonetheless, that is
wasted memory allocated and although quick, also a waste of time.
Am I being too obsessive? If not, would you like patches to correct these
as I find them, and if so, should I open a bug about this or just post
patches here (they are all likely to be a simple move of 1 or 2 lines)?
Sincerely,
Kean
