From: Vsevolod Stakhov Date: Sat, 25 Jul 2026 09:19:06 +0000 (+0100) Subject: [CritFix] http: fix shared body storage lifecycle X-Git-Tag: 4.1.3~19 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=f240cb00ebcf68cbab56a70571338b6c6cd6d7fa;p=thirdparty%2Frspamd.git [CritFix] http: fix shared body storage lifecycle The proxy cleared RSPAMD_HTTP_FLAG_SHMEM by hand before installing a new body. The following rspamd_http_message_set_body() then cleaned up the storage as if it were an fstring and free(3)'d the refcounted rspamd_storage_shmem the union aliases, whilst the segment descriptor and its mapping were leaked. Add rspamd_http_message_drop_shared_body() which releases the shared storage whilst the flags still describe it, and use it in all three places. storage_cleanup() skipped a segment sitting on descriptor 0, leaking both the descriptor and the mapping whenever the kernel handed us that one. Releasing it uncovered the reason it looked harmless: a message can carry the shared flag before a segment exists for it (a body-less copy inherits the flag from its origin), and the zero then read as a valid descriptor and closed whatever occupied slot 0 - in a proxy worker that is the accepted client socket. Initialise shm_fd to -1 in rspamd_http_new_message() so it is only ever valid for a real segment, then release it properly. Whilst here: reset `name` after releasing it so the next cleanup cannot release it twice, drop the fstat(2) hidden inside g_assert(), reject non-regular and empty descriptors in set_body_from_fd(), guard the length overflow in grow_body() and validate that the body of a copied message really fits the segment it is mapped from. --- diff --git a/src/libserver/http/http_connection.c b/src/libserver/http/http_connection.c index a3971b175c..347a7f5486 100644 --- a/src/libserver/http/http_connection.c +++ b/src/libserver/http/http_connection.c @@ -1501,11 +1501,15 @@ rspamd_http_connection_copy_msg(struct rspamd_http_message *msg, GError **err) if (msg->body_buf.len > 0) { if (msg->flags & RSPAMD_HTTP_FLAG_SHMEM) { + gsize body_off = 0; + /* Avoid copying by just mapping a shared segment */ new_msg->flags |= RSPAMD_HTTP_FLAG_SHMEM_IMMUTABLE; storage = &new_msg->body_buf.c; + storage->shared.name = NULL; storage->shared.shm_fd = dup(msg->body_buf.c.shared.shm_fd); + new_msg->body_buf.str = MAP_FAILED; if (storage->shared.shm_fd == -1) { rspamd_http_message_unref(new_msg); @@ -1525,6 +1529,24 @@ rspamd_http_connection_copy_msg(struct rspamd_http_message *msg, GError **err) return NULL; } + if (RSPAMD_HTTP_BODY_IS_MAPPED(msg) && + msg->body_buf.begin >= msg->body_buf.str) { + body_off = msg->body_buf.begin - msg->body_buf.str; + } + + if (st.st_size <= 0 || + (gsize) st.st_size < body_off || + msg->body_buf.len > (gsize) st.st_size - body_off) { + g_set_error(err, http_error_quark(), EINVAL, + "shmem fd %d does not fit the body: %zu bytes at " + "offset %zu of %lld", + storage->shared.shm_fd, msg->body_buf.len, + body_off, (long long) st.st_size); + rspamd_http_message_unref(new_msg); + + return NULL; + } + /* We don't own segment, so do not try to touch it */ if (msg->body_buf.c.shared.name) { @@ -1545,10 +1567,9 @@ rspamd_http_connection_copy_msg(struct rspamd_http_message *msg, GError **err) return NULL; } - new_msg->body_buf.begin = new_msg->body_buf.str; + new_msg->body_buf.begin = new_msg->body_buf.str + body_off; new_msg->body_buf.len = msg->body_buf.len; - new_msg->body_buf.begin = new_msg->body_buf.str + - (msg->body_buf.begin - msg->body_buf.str); + new_msg->body_buf.allocated_len = st.st_size; } else { old_body = rspamd_http_message_get_body(msg, &old_len); @@ -1874,7 +1895,14 @@ rspamd_http_detach_shared(struct rspamd_http_message *msg) { rspamd_fstring_t *cpy_str; - cpy_str = rspamd_fstring_new_init(msg->body_buf.begin, msg->body_buf.len); + if (msg->body_buf.begin != NULL && msg->body_buf.len > 0) { + cpy_str = rspamd_fstring_new_init(msg->body_buf.begin, + msg->body_buf.len); + } + else { + cpy_str = rspamd_fstring_new(); + } + rspamd_http_message_set_body_from_fstring_steal(msg, cpy_str); } @@ -2316,14 +2344,20 @@ rspamd_http_connection_write_message_common(struct rspamd_http_connection *conn, allow_shared = FALSE; } else { + gsize shm_offset = 0; + + if (RSPAMD_HTTP_BODY_IS_MAPPED(msg) && + msg->body_buf.begin >= msg->body_buf.str) { + shm_offset = msg->body_buf.begin - msg->body_buf.str; + } + /* Insert new headers */ rspamd_http_message_add_header(msg, "Shm", msg->body_buf.c.shared.name->shm_name); - rspamd_snprintf(tmpbuf, sizeof(tmpbuf), "%d", - (int) (msg->body_buf.begin - msg->body_buf.str)); + rspamd_snprintf(tmpbuf, sizeof(tmpbuf), "%uz", shm_offset); rspamd_http_message_add_header(msg, "Shm-Offset", tmpbuf); - rspamd_snprintf(tmpbuf, sizeof(tmpbuf), "%z", + rspamd_snprintf(tmpbuf, sizeof(tmpbuf), "%uz", msg->body_buf.len); rspamd_http_message_add_header(msg, "Shm-Length", tmpbuf); diff --git a/src/libserver/http/http_message.c b/src/libserver/http/http_message.c index b27b64caae..3d2fa393a0 100644 --- a/src/libserver/http/http_message.c +++ b/src/libserver/http/http_message.c @@ -40,6 +40,14 @@ rspamd_http_new_message(enum rspamd_http_message_type type) new->type = type; new->method = HTTP_INVALID; new->headers = kh_init(rspamd_http_headers_hash); + /* + * `shm_fd` lives past the pointer that `normal` shares the union with, so + * initialising it here is safe for both storage kinds. It must never be + * left as a valid looking zero: a message can be flagged as shared before + * a segment is actually created for it, and the cleanup would then close a + * completely unrelated descriptor 0. + */ + new->body_buf.c.shared.shm_fd = -1; REF_INIT_RETAIN(new, rspamd_http_message_free); @@ -204,6 +212,11 @@ rspamd_http_message_set_body(struct rspamd_http_message *msg, rspamd_http_message_storage_cleanup(msg); if (msg->flags & RSPAMD_HTTP_FLAG_SHMEM) { + if (len == G_MAXSIZE) { + /* Unknown length is treated as an empty body */ + len = 0; + } + storage->shared.name = g_malloc(sizeof(*storage->shared.name)); REF_INIT_RETAIN(storage->shared.name, rspamd_http_shname_dtor); #ifdef HAVE_SANE_SHMEM @@ -225,7 +238,7 @@ rspamd_http_message_set_body(struct rspamd_http_message *msg, return FALSE; } - if (len != 0 && len != G_MAXSIZE) { + if (len != 0) { if (ftruncate(storage->shared.shm_fd, len) == -1) { return FALSE; } @@ -303,6 +316,8 @@ rspamd_http_message_set_body_from_fd(struct rspamd_http_message *msg, storage = &msg->body_buf.c; msg->flags |= RSPAMD_HTTP_FLAG_SHMEM | RSPAMD_HTTP_FLAG_SHMEM_IMMUTABLE; + /* We do not own the segment, so there is no name to unlink afterwards */ + storage->shared.name = NULL; storage->shared.shm_fd = dup(fd); msg->body_buf.str = MAP_FAILED; @@ -314,6 +329,11 @@ rspamd_http_message_set_body_from_fd(struct rspamd_http_message *msg, return FALSE; } + if (!S_ISREG(st.st_mode) || st.st_size <= 0) { + /* Nothing that can be mapped: not a regular file or an empty one */ + return FALSE; + } + msg->body_buf.str = mmap(NULL, st.st_size, PROT_READ, MAP_SHARED, storage->shared.shm_fd, 0); @@ -384,6 +404,11 @@ rspamd_http_message_grow_body(struct rspamd_http_message *msg, gsize len) return FALSE; } + if (len > G_MAXSIZE - msg->body_buf.len) { + /* Integer overflow in the requested size */ + return FALSE; + } + if (fstat(storage->shared.shm_fd, &st) == -1) { return FALSE; } @@ -394,8 +419,9 @@ rspamd_http_message_grow_body(struct rspamd_http_message *msg, gsize len) newlen = rspamd_fstring_suggest_size(msg->body_buf.len, st.st_size, len); /* Unmap as we need another size of segment */ - if (msg->body_buf.str != MAP_FAILED) { + if (RSPAMD_HTTP_BODY_IS_MAPPED(msg)) { munmap(msg->body_buf.str, st.st_size); + msg->body_buf.str = MAP_FAILED; } if (ftruncate(storage->shared.shm_fd, newlen) == -1) { @@ -485,11 +511,21 @@ void rspamd_http_message_storage_cleanup(struct rspamd_http_message *msg) if (msg->flags & RSPAMD_HTTP_FLAG_SHMEM) { storage = &msg->body_buf.c; - if (storage->shared.shm_fd > 0) { - g_assert(fstat(storage->shared.shm_fd, &st) != -1); - - if (msg->body_buf.str != MAP_FAILED) { - munmap(msg->body_buf.str, st.st_size); + if (storage->shared.shm_fd >= 0) { + if (RSPAMD_HTTP_BODY_IS_MAPPED(msg)) { + /* + * We map the whole segment, hence its current size is the + * mapping length. If fstat fails somehow, we have no reliable + * length to unmap, so we have to leak the mapping instead of + * unmapping a wrong range. + */ + if (fstat(storage->shared.shm_fd, &st) != -1) { + munmap(msg->body_buf.str, st.st_size); + } + else { + msg_err("cannot fstat shmem fd %d: %s; mapping is leaked", + storage->shared.shm_fd, strerror(errno)); + } } close(storage->shared.shm_fd); @@ -499,6 +535,12 @@ void rspamd_http_message_storage_cleanup(struct rspamd_http_message *msg) REF_RELEASE(storage->shared.name); } + /* + * `name` shares the storage with `normal` (it is a union), so it must + * be reset unconditionally: leaving a dangling pointer here would make + * the next cleanup release it for the second time + */ + storage->shared.name = NULL; storage->shared.shm_fd = -1; msg->body_buf.str = MAP_FAILED; } @@ -513,6 +555,23 @@ void rspamd_http_message_storage_cleanup(struct rspamd_http_message *msg) msg->body_buf.len = 0; } +void rspamd_http_message_drop_shared_body(struct rspamd_http_message *msg) +{ + if (!(msg->flags & RSPAMD_HTTP_FLAG_SHMEM)) { + return; + } + + /* Cleanup whilst the flags still describe the storage we actually have */ + rspamd_http_message_storage_cleanup(msg); + + msg->flags &= ~(RSPAMD_HTTP_FLAG_SHMEM | RSPAMD_HTTP_FLAG_SHMEM_IMMUTABLE); + msg->body_buf.c.normal = NULL; + msg->body_buf.begin = NULL; + msg->body_buf.str = NULL; + msg->body_buf.len = 0; + msg->body_buf.allocated_len = 0; +} + void rspamd_http_message_free(struct rspamd_http_message *msg) { struct rspamd_http_header *hdr, *hcur, *hcurtmp; diff --git a/src/libserver/http/http_message.h b/src/libserver/http/http_message.h index 0b1c9441c6..6d58e6571f 100644 --- a/src/libserver/http/http_message.h +++ b/src/libserver/http/http_message.h @@ -237,6 +237,18 @@ struct rspamd_storage_shmem *rspamd_http_message_shmem_ref(struct rspamd_http_me */ void rspamd_http_message_shmem_unref(struct rspamd_storage_shmem *p); +/** + * Release shared memory storage of a message (if any) and switch it back to the + * ordinary heap storage. + * + * The body content is NOT preserved, so this is merely a way to say "forget the + * shared body, I'm going to set a new one". Clearing RSPAMD_HTTP_FLAG_SHMEM + * manually instead of calling this function leaks the mapping and the segment + * descriptor, and makes the next cleanup treat the shmem storage as an fstring. + * @param msg + */ +void rspamd_http_message_drop_shared_body(struct rspamd_http_message *msg); + /** * Returns message's flags * @param msg diff --git a/src/libserver/http/http_private.h b/src/libserver/http/http_private.h index 8613a4366b..3d6b545367 100644 --- a/src/libserver/http/http_private.h +++ b/src/libserver/http/http_private.h @@ -25,6 +25,8 @@ #include "upstream.h" #include "khash.h" +#include + #ifdef __cplusplus extern "C" { #endif @@ -94,6 +96,14 @@ struct rspamd_http_message { ref_entry_t ref; }; +/* + * True if the message body currently lives in a shared mapping that we own and + * hence have to unmap. `str` is NULL for a message with no body yet and + * MAP_FAILED once the storage has been cleaned up or a mapping attempt failed. + */ +#define RSPAMD_HTTP_BODY_IS_MAPPED(msg) \ + ((msg)->body_buf.str != NULL && (msg)->body_buf.str != (char *) MAP_FAILED) + struct rspamd_keepalive_hash_key { rspamd_inet_addr_t *addr; char *host; diff --git a/src/rspamd_proxy.c b/src/rspamd_proxy.c index b8d1c60e7b..45c33262d6 100644 --- a/src/rspamd_proxy.c +++ b/src/rspamd_proxy.c @@ -119,7 +119,7 @@ struct rspamd_http_mirror { gboolean local; gboolean compress; gboolean ssl; - gboolean keepalive; /* Whether to use keepalive for this mirror */ + gboolean keepalive; /* Whether to use keepalive for this mirror */ gboolean follow_master; /* Tie mirror lifetime to master upstream */ enum rspamd_proxy_log_tag_type log_tag_type; ucl_object_t *extra_headers; @@ -2070,7 +2070,7 @@ proxy_open_mirror_connections(struct rspamd_proxy_session *session) } else { if (session->fname) { - msg->flags &= ~RSPAMD_HTTP_FLAG_SHMEM; + rspamd_http_message_drop_shared_body(msg); rspamd_http_message_set_body(msg, session->map, session->map_len); } @@ -2232,7 +2232,7 @@ proxy_open_mirror_connections(struct rspamd_proxy_session *session) } else { if (session->fname) { - msg->flags &= ~RSPAMD_HTTP_FLAG_SHMEM; + rspamd_http_message_drop_shared_body(msg); rspamd_http_message_set_body(msg, session->map, session->map_len); } @@ -3021,7 +3021,7 @@ proxy_send_master_message(struct rspamd_proxy_session *session) } else { if (session->fname) { - msg->flags &= ~RSPAMD_HTTP_FLAG_SHMEM; + rspamd_http_message_drop_shared_body(msg); rspamd_http_message_set_body(msg, session->map, session->map_len); }