On Sat, Jan 27, 2018 at 3:14 PM, Yann Ylavic <[email protected]> wrote:
>
> So I wonder if we'd better:
> 1. have a constant file name (no generation suffix) for all systems,
> 2. not unlink/remove the SHM file on pconf cleanup,
> 2'. eventually unlink the ones unused in ++generation (e.g. retained
> global list),
> 3. unlink all on pglobal cleanup.
>
> Now we'd have a working shm_attach() not only for persisted slotmems,
> while shm_create() would only be called for new SHMs which we know can
> be pre-removed (to work around crashes leaving them registered
> somewhere).
> Also, if attach succeeds on startup (first generation) but the SHM is
> not persisted (an old crash still), we can possibly pass the sizes
> checks and re-create the SHM regardless.
>
> WDYT?
Something like the attached patch (it talks better sometimes...).
Not even tested of course :)
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1821739)
+++ modules/proxy/proxy_util.c (working copy)
@@ -1370,7 +1370,9 @@ static apr_status_t connection_cleanup(void *theco
}
/* determine if the connection need to be closed */
- if (!worker->s->is_address_reusable || worker->s->disablereuse) {
+ if (!worker->s->is_address_reusable
+ || worker->s->disablereuse
+ || conn->uds_path) {
apr_pool_t *p = conn->pool;
apr_pool_clear(p);
conn = apr_pcalloc(p, sizeof(proxy_conn_rec));
@@ -2295,24 +2297,14 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
* to check host and port on the conn and be careful about
* spilling the cached addr from the worker.
*/
- uds_path = (*worker->s->uds_path ? worker->s->uds_path : apr_table_get(r->notes, "uds_path"));
+ uds_path = *worker->s->uds_path ? worker->s->uds_path
+ : apr_table_get(r->notes, "uds_path");
if (uds_path) {
- if (conn->uds_path == NULL) {
- /* use (*conn)->pool instead of worker->cp->pool to match lifetime */
- conn->uds_path = apr_pstrdup(conn->pool, uds_path);
- }
- if (conn->uds_path) {
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02545)
- "%s: has determined UDS as %s",
- uri->scheme, conn->uds_path);
- }
- else {
- /* should never happen */
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02546)
- "%s: cannot determine UDS (%s)",
- uri->scheme, uds_path);
-
- }
+ /* use conn->pool instead of worker->cp->pool to match lifetime */
+ conn->uds_path = apr_pstrdup(conn->pool, uds_path);
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02545)
+ "%s: has determined UDS as %s",
+ uri->scheme, conn->uds_path);
/*
* In UDS cases, some structs are NULL. Protect from de-refs
* and provide info for logging at the same time.
@@ -2829,6 +2821,7 @@ PROXY_DECLARE(int) ap_proxy_connect_backend(const
break;
}
conn->connection = NULL;
+ conn->close = 1;
rv = ap_proxy_connect_uds(newsock, conn->uds_path, conn->scpool);
if (rv != APR_SUCCESS) {
Index: modules/slotmem/mod_slotmem_shm.c
===================================================================
--- modules/slotmem/mod_slotmem_shm.c (revision 1821739)
+++ modules/slotmem/mod_slotmem_shm.c (working copy)
@@ -47,7 +47,7 @@ struct ap_slotmem_instance_t {
int fbased; /* filebased? */
void *shm; /* ptr to memory segment (apr_shm_t *) */
void *base; /* data set start */
- apr_pool_t *gpool; /* per segment global pool */
+ apr_pool_t *pool; /* per segment working pool */
char *inuse; /* in-use flag table*/
unsigned int *num_free; /* slot free count for this instance */
void *persist; /* persist dataset start */
@@ -64,7 +64,8 @@ struct ap_slotmem_instance_t {
*/
/* global pool and list of slotmem we are handling */
-static struct ap_slotmem_instance_t *globallistmem = NULL;
+static struct ap_slotmem_instance_t *globallistmem = NULL,
+ **retained_globallistmem = NULL;
static apr_pool_t *gpool = NULL;
#define DEFAULT_SLOTMEM_PREFIX "slotmem-shm-"
@@ -191,11 +192,11 @@ static void store_slotmem(ap_slotmem_instance_t *s
if (storename) {
rv = apr_file_open(&fp, storename, APR_CREATE | APR_READ | APR_WRITE,
- APR_OS_DEFAULT, slotmem->gpool);
+ APR_OS_DEFAULT, slotmem->pool);
if (APR_STATUS_IS_EEXIST(rv)) {
- apr_file_remove(storename, slotmem->gpool);
+ apr_file_remove(storename, slotmem->pool);
rv = apr_file_open(&fp, storename, APR_CREATE | APR_READ | APR_WRITE,
- APR_OS_DEFAULT, slotmem->gpool);
+ APR_OS_DEFAULT, slotmem->pool);
}
if (rv != APR_SUCCESS) {
return;
@@ -212,7 +213,7 @@ static void store_slotmem(ap_slotmem_instance_t *s
}
apr_file_close(fp);
if (rv != APR_SUCCESS) {
- apr_file_remove(storename, slotmem->gpool);
+ apr_file_remove(storename, slotmem->pool);
}
}
}
@@ -271,26 +272,35 @@ static apr_status_t restore_slotmem(void *ptr, con
return rv;
}
+static apr_status_t global_cleanup_slotmem(void *param)
+{
+ AP_DEBUG_ASSERT(retained_globallistmem != NULL);
+ *retained_globallistmem = NULL;
+
+ while (globallistmem) {
+ if (globallistmem->fbased) {
+ apr_shm_destroy(globallistmem->shm);
+ apr_shm_remove(globallistmem->name, globallistmem->pool);
+ apr_file_remove(globallistmem->name, globallistmem->pool);
+ }
+ globallistmem = globallistmem->next;
+ }
+
+ return APR_SUCCESS;
+}
+
static apr_status_t cleanup_slotmem(void *param)
{
- ap_slotmem_instance_t **mem = param;
+ AP_DEBUG_ASSERT(retained_globallistmem != NULL);
+ *retained_globallistmem = globallistmem;
- if (*mem) {
- ap_slotmem_instance_t *next = *mem;
- while (next) {
- if (AP_SLOTMEM_IS_PERSIST(next)) {
- store_slotmem(next);
- }
- apr_shm_destroy((apr_shm_t *)next->shm);
- if (next->fbased) {
- apr_shm_remove(next->name, next->gpool);
- apr_file_remove(next->name, next->gpool);
- }
- next = next->next;
+ while (globallistmem) {
+ if (AP_SLOTMEM_IS_PERSIST(globallistmem)) {
+ store_slotmem(globallistmem);
}
+ globallistmem = globallistmem->next;
}
- /* apr_pool_destroy(gpool); */
- globallistmem = NULL;
+
return APR_SUCCESS;
}
@@ -338,11 +348,11 @@ static apr_status_t slotmem_create(ap_slotmem_inst
apr_size_t size = AP_SLOTMEM_OFFSET + AP_UNSIGNEDINT_OFFSET +
(item_num * sizeof(char)) + basesize;
int persist = (type & AP_SLOTMEM_TYPE_PERSIST) != 0;
+ int generation = 0;
apr_status_t rv;
- if (gpool == NULL) {
- return APR_ENOSHMAVAIL;
- }
+ ap_mpm_query(AP_MPMQ_GENERATION, &generation);
+
if (slotmem_filenames(pool, name, &fname, persist ? &pname : NULL)) {
/* first try to attach to existing slotmem */
if (next) {
@@ -372,13 +382,13 @@ static apr_status_t slotmem_create(ap_slotmem_inst
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf, APLOGNO(02300)
"create %s: %"APR_SIZE_T_FMT"/%u", fname, item_size,
item_num);
- if (fbased) {
- rv = apr_shm_attach(&shm, fname, gpool);
+ if (fbased && generation > 1) {
+ rv = apr_shm_attach(&shm, fname, pool);
}
else {
rv = APR_EINVAL;
}
- if (rv == APR_SUCCESS) {
+ if (rv == APR_SUCCESS && persist) {
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, ap_server_conf, APLOGNO(02598)
"apr_shm_attach() succeeded");
@@ -401,10 +411,10 @@ static apr_status_t slotmem_create(ap_slotmem_inst
}
ptr += AP_SLOTMEM_OFFSET;
}
- else {
+ else if (rv != APR_SUCCESS) {
apr_size_t dsize = size - AP_SLOTMEM_OFFSET;
if (fbased) {
- apr_shm_remove(fname, gpool);
+ apr_shm_remove(fname, pool);
rv = apr_shm_create(&shm, size, fname, gpool);
}
else {
@@ -444,8 +454,7 @@ static apr_status_t slotmem_create(ap_slotmem_inst
}
/* For the chained slotmem stuff */
- res = (ap_slotmem_instance_t *) apr_pcalloc(gpool,
- sizeof(ap_slotmem_instance_t));
+ res = apr_pcalloc(gpool, sizeof(ap_slotmem_instance_t));
res->name = apr_pstrdup(gpool, fname);
res->pname = apr_pstrdup(gpool, pname);
res->fbased = fbased;
@@ -458,7 +467,7 @@ static apr_status_t slotmem_create(ap_slotmem_inst
ptr += AP_UNSIGNEDINT_OFFSET;
res->base = (void *)ptr;
res->desc = desc;
- res->gpool = gpool;
+ res->pool = pool;
res->next = NULL;
res->inuse = ptr + basesize;
if (globallistmem == NULL) {
@@ -485,9 +494,6 @@ static apr_status_t slotmem_attach(ap_slotmem_inst
apr_shm_t *shm;
apr_status_t rv;
- if (gpool == NULL) {
- return APR_ENOSHMAVAIL;
- }
if (!slotmem_filenames(pool, name, &fname, NULL)) {
return APR_ENOSHMAVAIL;
}
@@ -528,8 +534,7 @@ static apr_status_t slotmem_attach(ap_slotmem_inst
ptr += AP_SLOTMEM_OFFSET;
/* For the chained slotmem stuff */
- res = (ap_slotmem_instance_t *) apr_pcalloc(gpool,
- sizeof(ap_slotmem_instance_t));
+ res = apr_pcalloc(gpool, sizeof(ap_slotmem_instance_t));
res->name = apr_pstrdup(gpool, fname);
res->fbased = 1;
res->shm = shm;
@@ -538,7 +543,7 @@ static apr_status_t slotmem_attach(ap_slotmem_inst
ptr += AP_UNSIGNEDINT_OFFSET;
res->base = (void *)ptr;
res->desc = desc;
- res->gpool = gpool;
+ res->pool = pool;
res->inuse = ptr + (desc.size * desc.num);
res->next = NULL;
if (globallistmem == NULL) {
@@ -760,18 +765,23 @@ static const ap_slotmem_provider_t *slotmem_shm_ge
}
/* initialise the global pool */
-static void slotmem_shm_initgpool(apr_pool_t *p)
+static int pre_config(apr_pool_t *p, apr_pool_t *plog,
+ apr_pool_t *ptemp)
{
- gpool = p;
+ const char *retained_key = "mod_slotmem_shm";
+ retained_globallistmem = ap_retained_data_get(retained_key);
+ if (!retained_globallistmem) {
+ retained_globallistmem =
+ ap_retained_data_create(retained_key,
+ sizeof *retained_globallistmem);
+ apr_pool_cleanup_register(ap_pglobal, NULL, global_cleanup_slotmem,
+ apr_pool_cleanup_null);
+ }
+ globallistmem = *retained_globallistmem;
+ gpool = ap_pglobal;
+ return OK;
}
-/* Add the pool_clean routine */
-static void slotmem_shm_initialize_cleanup(apr_pool_t *p)
-{
- apr_pool_cleanup_register(p, &globallistmem, cleanup_slotmem,
- apr_pool_cleanup_null);
-}
-
/*
* Make sure the shared memory is cleaned
*/
@@ -778,17 +788,11 @@ static const ap_slotmem_provider_t *slotmem_shm_ge
static int post_config(apr_pool_t *p, apr_pool_t *plog, apr_pool_t *ptemp,
server_rec *s)
{
- slotmem_shm_initialize_cleanup(p);
+ apr_pool_cleanup_register(p, NULL, cleanup_slotmem,
+ apr_pool_cleanup_null);
return OK;
}
-static int pre_config(apr_pool_t *p, apr_pool_t *plog,
- apr_pool_t *ptemp)
-{
- slotmem_shm_initgpool(p);
- return OK;
-}
-
static void ap_slotmem_shm_register_hook(apr_pool_t *p)
{
const ap_slotmem_provider_t *storage = slotmem_shm_getstorage();
Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1821739)
+++ server/protocol.c (working copy)
@@ -225,6 +225,11 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
int fold = flags & AP_GETLINE_FOLD;
int crlf = flags & AP_GETLINE_CRLF;
+ if (!n) {
+ /* Can't work since we always nul terminate */
+ return APR_ENOSPC;
+ }
+
/*
* Initialize last_char as otherwise a random value will be compared
* against APR_ASCII_LF at the end of the loop if bb only contains
@@ -238,7 +243,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
rv = ap_get_brigade(r->proto_input_filters, bb, AP_MODE_GETLINE,
APR_BLOCK_READ, 0);
if (rv != APR_SUCCESS) {
- return rv;
+ goto cleanup;
}
/* Something horribly wrong happened. Someone didn't block!
@@ -245,7 +250,8 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
* (this also happens at the end of each keepalive connection)
*/
if (APR_BRIGADE_EMPTY(bb)) {
- return APR_EGENERAL;
+ rv = APR_EGENERAL;
+ goto cleanup;
}
for (e = APR_BRIGADE_FIRST(bb);
@@ -263,7 +269,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
rv = apr_bucket_read(e, &str, &len, APR_BLOCK_READ);
if (rv != APR_SUCCESS) {
- return rv;
+ goto cleanup;
}
if (len == 0) {
@@ -276,17 +282,8 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
/* Would this overrun our buffer? If so, we'll die. */
if (n < bytes_handled + len) {
- *read = bytes_handled;
- if (*s) {
- /* ensure this string is NUL terminated */
- if (bytes_handled > 0) {
- (*s)[bytes_handled-1] = '\0';
- }
- else {
- (*s)[0] = '\0';
- }
- }
- return APR_ENOSPC;
+ rv = APR_ENOSPC;
+ goto cleanup;
}
/* Do we have to handle the allocation ourselves? */
@@ -329,19 +326,15 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
}
}
- if (crlf && (last_char <= *s || last_char[-1] != APR_ASCII_CR)) {
- *last_char = '\0';
- bytes_handled = last_char - *s;
- *read = bytes_handled;
- return APR_EINVAL;
- }
-
/* Now NUL-terminate the string at the end of the line;
* if the last-but-one character is a CR, terminate there */
if (last_char > *s && last_char[-1] == APR_ASCII_CR) {
last_char--;
}
- *last_char = '\0';
+ else if (crlf) {
+ rv = APR_EINVAL;
+ goto cleanup;
+ }
bytes_handled = last_char - *s;
/* If we're folding, we have more work to do.
@@ -361,7 +354,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
rv = ap_get_brigade(r->proto_input_filters, bb, AP_MODE_SPECULATIVE,
APR_BLOCK_READ, 1);
if (rv != APR_SUCCESS) {
- return rv;
+ goto cleanup;
}
if (APR_BRIGADE_EMPTY(bb)) {
@@ -378,7 +371,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
rv = apr_bucket_read(e, &str, &len, APR_BLOCK_READ);
if (rv != APR_SUCCESS) {
apr_brigade_cleanup(bb);
- return rv;
+ goto cleanup;
}
/* Found one, so call ourselves again to get the next line.
@@ -395,10 +388,8 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
if (c == APR_ASCII_BLANK || c == APR_ASCII_TAB) {
/* Do we have enough space? We may be full now. */
if (bytes_handled >= n) {
- *read = n;
- /* ensure this string is terminated */
- (*s)[n-1] = '\0';
- return APR_ENOSPC;
+ rv = APR_ENOSPC;
+ goto cleanup;
}
else {
apr_size_t next_size, next_len;
@@ -411,7 +402,6 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
tmp = NULL;
}
else {
- /* We're null terminated. */
tmp = last_char;
}
@@ -420,7 +410,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
rv = ap_rgetline_core(&tmp, next_size,
&next_len, r, 0, bb);
if (rv != APR_SUCCESS) {
- return rv;
+ goto cleanup;
}
if (do_alloc && next_len > 0) {
@@ -434,7 +424,7 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
memcpy(new_buffer, *s, bytes_handled);
/* copy the new line, including the trailing null */
- memcpy(new_buffer + bytes_handled, tmp, next_len + 1);
+ memcpy(new_buffer + bytes_handled, tmp, next_len);
*s = new_buffer;
}
@@ -447,8 +437,21 @@ AP_DECLARE(apr_status_t) ap_rgetline_core(char **s
}
}
}
+
+cleanup:
+ if (bytes_handled >= n) {
+ bytes_handled = n - 1;
+ }
+ if (*s) {
+ /* ensure this string is NUL terminated */
+ (*s)[bytes_handled] = '\0';
+ }
*read = bytes_handled;
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+
/* PR#43039: We shouldn't accept NULL bytes within the line */
if (strlen(*s) < bytes_handled) {
return APR_EINVAL;
@@ -487,6 +490,10 @@ AP_DECLARE(int) ap_getline(char *s, int n, request
apr_size_t len;
apr_bucket_brigade *tmp_bb;
+ if (n < 0) {
+ return n;
+ }
+
tmp_bb = apr_brigade_create(r->pool, r->connection->bucket_alloc);
rv = ap_rgetline(&tmp_s, n, &len, r, flags, tmp_bb);
apr_brigade_destroy(tmp_bb);
