Re: apr_pollcb

[Paul Querna]

Can you describe "lagging" in more detail?

None of the poll related code has a high rate of change (except for the
relatively new z/OS backend):

https://github.com/apache/apr/tree/trunk/poll/unix

Also are you looking specifically on Linux? (epoll backend?) or others

On Mon, Mar 9, 2015 at 11:04 AM, Jim Jagielski  wrote:

> As far as I can tell, there are no real consumers of apr_pollcb_*()
> other than Motorz (and Simple)... Not sure, but it looks like the
> pollcb stuff may be lagging, since I'm getting some weird behavior
> when using that impl. So I'll likely be moving Motorz off of
> pollcb and use the "traditional" impl which has been proven in
> Event.
>
>

Re: PR 53435, r101624, mod_ssl: error strings can't be loaded again once?

[Jan Kaluža]

On 11/11/2014 02:32 PM, Jan Kaluža wrote:

Hi,

latest comment in PR 53435 shows that memory leak in mod_ssl which
happens during graceful restarts can be caused by r101624. Since this
commit is 11 years old, I wanted to ask people here, if following is
still true with current OpenSSL:


Hi,

little follow-up here, we have memory leak on reload in 
ENGINE_load_builtin_engines which is caused by 
. Again, this is 6 years old commit which 
may or may not be true. Does anyone know if it's still valid?



+/* Also don't call CRYPTO_cleanup_all_ex_data here; any registered
+ * ex_data indices may have been cached in static variables in
+ * OpenSSL; removing them may cause havoc.  Notably, with OpenSSL
+ * versions >= 0.9.8f, COMP_CTX cleanups would not be run, which
+ * could result in a per-connection memory leak (!). */
+


Regards,
Jan Kaluza


@@ -255,7 +255,11 @@ static apr_status_t ssl_cleanup_pre_config(void
*data)
#endif
#endif
ERR_remove_state(0);
- ERR_free_strings();
+
+ /* Don't call ERR_free_strings here; ERR_load_*_strings only
+ * actually load the error strings once per process due to static
+ * variable abuse in OpenSSL. */
+
/*
* TODO: determine somewhere we can safely shove out diagnostics
* (when enabled) at this late stage in the game:


Last comment in PR 53435 showed that leaks disappeared after reverting
this patch and it does not seem to break anything here.

Regards,
Jan Kaluza

Shouldn't ap_die() do nothing -inconditionally- when a response was already generated?

[Yann Ylavic]

That is, not only when a hook/handler returns AP_FILTER_ERROR.

Graham (initially) and I made some changes to trunk (backport proposed
in r1665737) so that no internal module should return anything but
AP_FILTER_ERROR in this case, but we can't do much for third-party
modules.

That way ap_die() could become a safe gard against unintentional
double responses (hence HRS) for the same request, in any case since
it is always called at the end.

Something like:

Index: modules/http/http_request.c
===
--- modules/http/http_request.c(revision 1665647)
+++ modules/http/http_request.c(working copy)
@@ -75,6 +75,7 @@ static void update_r_in_filters(ap_filter_t *f,

 static void ap_die_r(int type, request_rec *r, int recursive_error)
 {
+ap_filter_t *next;
 char *custom_response;
 request_rec *r_1st_err = r;

@@ -83,38 +84,34 @@ static void ap_die_r(int type, request_rec *r, int
 return;
 }

+/*
+ * Check if we still have the ap_http_header_filter in place. If
+ * this is the case we should not ignore the error here because
+ * it means that we have not sent any response at all. Otherwise,
+ * don't send two responses for the same request.
+ */
+next = r->output_filters;
+while (next && (next->frec != ap_http_header_filter_handle)) {
+next = next->next;
+}
+if (!next) {
+return;
+}
+
+/*
+ * AP_FILTER_ERROR or any invalid status, send an internal server
+ * error instead.
+ */
 if (!ap_is_HTTP_VALID_RESPONSE(type)) {
-ap_filter_t *next;
-
-/*
- * Check if we still have the ap_http_header_filter in place. If
- * this is the case we should not ignore the error here because
- * it means that we have not sent any response at all and never
- * will. This is bad. Sent an internal server error instead.
- */
-next = r->output_filters;
-while (next && (next->frec != ap_http_header_filter_handle)) {
-   next = next->next;
-}
-
-/*
- * If next != NULL then we left the while above because of
- * next->frec == ap_http_header_filter
- */
-if (next) {
-if (type != AP_FILTER_ERROR) {
-ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01579)
-  "Invalid response status %i", type);
-}
-else {
-ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02831)
-  "Response from AP_FILTER_ERROR");
-}
-type = HTTP_INTERNAL_SERVER_ERROR;
-}
-else {
-return;
-}
+if (type != AP_FILTER_ERROR) {
+ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01579)
+  "Invalid response status %i", type);
+}
+else {
+ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02831)
+  "Response from AP_FILTER_ERROR");
+}
+type = HTTP_INTERNAL_SERVER_ERROR;
 }

 /*
--

This is not a great cycles loss since this path is not a fast one
already (!OK && !DONE => ap_send_error_response() or
ap_internal_redirect()), and the inconditional loop is not really
heavy...

Thoughts?

Regards,
Yann.