Two review findings.
rspamd_shmem_segment_map() mapped the selected window and copied out of
it. The backing object belongs to the client, so an ftruncate landing
between the fstat and the memcpy raised SIGBUS inside the copy and took
the worker down; the mapping was only dropped afterwards, so the earlier
regression test, which resized the object once the call had already
returned, never exercised that window at all.
Read the window straight into pool storage instead. A read cannot fault:
a concurrent truncation merely returns fewer bytes, and the payload
length now reflects what was actually read rather than what was asked
for. Short reads and EINTR are handled in the loop.
POSIX shared memory descriptors do not accept positional reads on every
platform - macOS reports ESPIPE and some BSDs ENODEV - so the mapping
path is kept as a fallback for exactly those descriptors. On Linux,
where shared memory objects live on tmpfs and HAVE_SANE_SHMEM is always
set, the read path is the one taken and the race is closed.
proxy_strip_query_args() rebuilt the url from a prefix of
u.field_data[UF_QUERY].off bytes. That offset addresses the first byte
after the '?', so the prefix already carried the delimiter and the
appended one produced '/checkv2??From=...'. Since the proxy turns query
arguments into request headers at the upstream, the first surviving
argument was then parsed as '?From' and its value silently lost. Stop
the prefix one byte short. The previous test stripped every argument
present, so nothing survived and the corruption stayed invisible.
Both regressions are now covered. A thread toggles the size of a backing
object while the snapshot is taken repeatedly, which faults on the old
code and passes on the new one, and a proxy request now carries an
ordinary query argument alongside a privileged one and asserts the
ordinary one reaches the upstream intact.
return TRUE;
}
+enum rspamd_snapshot_result {
+ RSPAMD_SNAPSHOT_OK = 0,
+ RSPAMD_SNAPSHOT_ERROR,
+ /* This descriptor does not support positional reads at all */
+ RSPAMD_SNAPSHOT_UNSUPPORTED,
+};
+
+/*
+ * Reads `len` bytes at `offset` into `buf`, coping with short reads and EINTR.
+ *
+ * Unlike a copy out of a mapping, a read can never fault: if the object is
+ * truncated while we are reading it we merely get fewer bytes than we asked
+ * for, which the caller reflects in the payload length.
+ */
+static enum rspamd_snapshot_result
+rspamd_task_pread_snapshot(int fd, char *buf, gsize len, off_t offset,
+ gsize *read_len)
+{
+ gsize total = 0;
+
+ while (total < len) {
+ ssize_t r = pread(fd, buf + total, len - total,
+ offset + (off_t) total);
+
+ if (r > 0) {
+ total += (gsize) r;
+ }
+ else if (r == 0) {
+ /* Truncated under us, whatever we have got is all there is */
+ break;
+ }
+ else if (errno == EINTR) {
+ continue;
+ }
+ else if (total == 0 && (errno == ESPIPE || errno == ENODEV ||
+ errno == EINVAL || errno == ENOTSUP ||
+ errno == EOPNOTSUPP)) {
+ /*
+ * POSIX shared memory descriptors cannot be read on every platform
+ * (macOS returns ESPIPE, some BSDs ENODEV), so the caller has to
+ * fall back to mapping the window there.
+ */
+ return RSPAMD_SNAPSHOT_UNSUPPORTED;
+ }
+ else {
+ *read_len = total;
+
+ return RSPAMD_SNAPSHOT_ERROR;
+ }
+ }
+
+ *read_len = total;
+
+ return RSPAMD_SNAPSHOT_OK;
+}
+
struct rspamd_shmem_segment *
rspamd_shmem_segment_map(rspamd_mempool_t *pool,
const rspamd_ftok_t *name_tok,
char namebuf[PATH_MAX];
const char *name = namebuf;
gulong offset = 0, length = 0;
- gsize page_size, aligned_offset, delta, map_len;
+ gsize page_size, aligned_offset, delta, map_len, nread = 0;
struct stat st;
int fd;
gpointer map;
char *data;
+ enum rspamd_snapshot_result res;
struct rspamd_shmem_segment *seg;
#ifdef HAVE_SANE_SHMEM
const char *ft = "shm";
}
/*
- * Map merely the window that is really needed: the offset is rounded down
- * to a page boundary and the length is extended by the very same delta, so
- * that a small payload inside a huge object never maps that whole object.
+ * The backing object belongs to the client and it can be truncated or
+ * rewritten at any moment, so the parser must never be handed a live
+ * mapping. Read the window straight into pool storage instead of copying it
+ * out of one: a concurrent ftruncate between the fstat above and the copy
+ * would raise SIGBUS on a mapping and take the whole worker down, whereas a
+ * read merely returns fewer bytes.
+ */
+ data = rspamd_mempool_alloc(pool, length);
+ res = rspamd_task_pread_snapshot(fd, data, length, (off_t) offset, &nread);
+
+ if (res == RSPAMD_SNAPSHOT_ERROR) {
+ g_set_error(err, rspamd_task_quark(), RSPAMD_PROTOCOL_ERROR,
+ "cannot read %s segment (%s): %s", ft, name,
+ strerror(errno));
+ close(fd);
+
+ return NULL;
+ }
+
+ if (res == RSPAMD_SNAPSHOT_OK) {
+ close(fd);
+ /* The object may have shrunk under us, so trust what we really got */
+ seg->data_len = nread;
+ seg->data = data;
+
+ return seg;
+ }
+
+ /*
+ * This descriptor cannot be read positionally, which happens for POSIX
+ * shared memory on some platforms, so fall back to mapping. Map merely the
+ * window that is really needed: the offset is rounded down to a page
+ * boundary and the length is extended by the very same delta, so that a
+ * small payload inside a huge object never maps that whole object.
*/
page_size = (gsize) sysconf(_SC_PAGESIZE);
return NULL;
}
- /*
- * The backing object belongs to the client and it can be truncated or
- * rewritten at any moment, so a live mapping handed over to the parser
- * could fault later on. Snapshot the window into the pool and drop both the
- * mapping and the descriptor right away.
- */
- data = rspamd_mempool_alloc(pool, length);
memcpy(data, (const char *) map + delta, length);
munmap(map, map_len);
close(fd);
return;
}
+ if (u.field_data[UF_QUERY].off == 0) {
+ /* A query always follows a '?', so this cannot happen; be defensive */
+ g_hash_table_unref(query_args);
+
+ return;
+ }
+
+ /*
+ * UF_QUERY.off addresses the first byte *after* the '?', so the prefix has
+ * to stop one byte short of it: copying up to `off` would keep the original
+ * delimiter and the one appended below would produce a second one.
+ */
new_url = rspamd_fstring_new_init(RSPAMD_FSTRING_DATA(msg->url),
- u.field_data[UF_QUERY].off);
+ u.field_data[UF_QUERY].off - 1);
new_url = rspamd_fstring_append(new_url, "?", 1);
g_hash_table_iter_init(&it, query_args);
Expect Symbol SIMPLE_TEST
Expect Symbol With Exact Options FILE_SHM_PROBE none
+Ordinary query arguments survive the stripping of a privileged one
+ [Documentation] Stripping Shm means rebuilding the URL around it, and every
+ ... surviving argument becomes a request header at the upstream.
+ ... An argument that shared the URL with a stripped one must
+ ... therefore arrive under its own name: a rebuild that kept the
+ ... original '?' as well as the appended one renames it to
+ ... '?From', which silently drops the envelope sender instead.
+ ${data} = Get Binary File ${MESSAGE}
+ # From is deliberately *not* sent as a header: the query argument is the only
+ # source of it, so a mangled name cannot be masked by a surviving header
+ ${headers} = Create Dictionary Queue-Id=${TEST NAME}
+ ... Rcpt=qsurvive-rcpt@example.net
+ @{result} = HTTP Status And Reason POST ${RSPAMD_LOCAL_ADDR} ${RSPAMD_PORT_PROXY}
+ ... /checkv2?From=qsurvive@example.net&Shm=${SHM_NAME} ${data} ${headers}
+ Should Be Equal As Integers ${result}[0] 200
+ ${json} = Evaluate __import__('json').loads($result[2])
+ Set Test Variable ${SCAN_RESULT} ${json}
+ Expect Symbol SIMPLE_TEST
+ Expect Symbol With Exact Options REQUEST_ARG_PROBE From=qsurvive@example.net
+ # And the privileged one really was stripped, so this is not a case of the
+ # whole query having been forwarded untouched
+ Expect Symbol With Exact Options FILE_SHM_PROBE none
+
Client Shm headers cannot override a permissive proxy's own triplet
[Documentation] The other proxy worker does forward through shared memory,
... so a triplet really is generated on the upstream leg. It
-- value survive?" can be observed directly.
local privileged_headers = {'File', 'Path', 'Shm', 'Shm-Offset', 'Shm-Length'}
+-- Ordinary query arguments that share a URL with a stripped privileged one.
+-- Removing a privileged argument means rebuilding the URL around it, and the
+-- '?' delimiter is easy to duplicate while doing so, which renames the first
+-- surviving argument to '?From'. Both spellings are therefore reported, so
+-- that a mangled rebuild is named rather than merely missed.
+local ordinary_headers = {'From', '?From'}
+
+local function report_headers(task, names)
+ local seen = {}
+
+ for _, hname in ipairs(names) do
+ local hvalue = task:get_request_header(hname)
+ if hvalue then
+ seen[#seen + 1] = hname .. '=' .. tostring(hvalue)
+ end
+ end
+
+ if #seen == 0 then
+ return 'none'
+ end
+
+ return table.concat(seen, ';')
+end
+
rspamd_config:register_symbol({
name = 'FILE_SHM_PROBE',
score = 0.0,
callback = function(task)
- local seen = {}
-
- for _, hname in ipairs(privileged_headers) do
- local hvalue = task:get_request_header(hname)
- if hvalue then
- seen[#seen + 1] = hname .. '=' .. tostring(hvalue)
- end
- end
-
- if #seen == 0 then
- return true, 'none'
- end
+ return true, report_headers(task, privileged_headers)
+ end
+})
- return true, table.concat(seen, ';')
+rspamd_config:register_symbol({
+ name = 'REQUEST_ARG_PROBE',
+ score = 0.0,
+ callback = function(task)
+ return true, report_headers(task, ordinary_headers)
end
})
* - the payload handed to the caller is a private snapshot, therefore the
* client can resize the backing object afterwards without the parser
* ever seeing memory that can fault;
+ * - and, the sharp edge of that one, the snapshot is never *copied out of a
+ * mapping*: a client that truncates the object in the window between the
+ * validating fstat and the copy would otherwise raise SIGBUS inside the
+ * worker;
* - the name is sanitised before any syscall touches it and an overlong
* name is refused rather than silently truncated;
* - `Filename` is *not* a privileged control, whereas `File`, `Path`,
#include <sys/types.h>
#include <unistd.h>
+#include <atomic>
+#include <chrono>
#include <climits>
#include <cstdlib>
#include <cstring>
#include <string>
+#include <thread>
#include <vector>
namespace rspamd_task_input_test {
return true;
}
+ /*
+ * A second, writable descriptor for the very same object. A concurrent
+ * resizer needs one of its own, so that it never touches this fixture's
+ * bookkeeping from another thread.
+ */
+ int open_writable() const
+ {
+#ifdef HAVE_SANE_SHMEM
+ return shm_open(obj_name.c_str(), O_RDWR, 0);
+#else
+ return open(obj_name.c_str(), O_RDWR);
+#endif
+ }
+
/* Rewrites the whole object with the canonical pattern */
bool fill_pattern()
{
bool ok = false;
};
+/*
+ * Resizes a backing object from a second thread for as long as it is alive,
+ * which is how a client that owns the object behaves while the worker is
+ * reading it.
+ *
+ * The thread is bounded twice over -- by the stop flag and by a wall clock
+ * deadline -- and it is always joined from the destructor, so a failing
+ * assertion in the test body can neither leave it running nor hang the suite.
+ */
+class background_truncator {
+public:
+ background_truncator(const backing_object &obj, gsize big, gsize small)
+ {
+ fd = obj.open_writable();
+
+ if (fd == -1) {
+ return;
+ }
+
+ thr = std::thread([this, big, small]() {
+ auto deadline = std::chrono::steady_clock::now() +
+ std::chrono::seconds(10);
+
+ while (!stop.load(std::memory_order_relaxed) &&
+ std::chrono::steady_clock::now() < deadline) {
+ if (ftruncate(fd, (off_t) small) == -1 ||
+ ftruncate(fd, (off_t) big) == -1) {
+ break;
+ }
+
+ toggles.fetch_add(1, std::memory_order_relaxed);
+ }
+ });
+ }
+
+ background_truncator(const background_truncator &) = delete;
+ background_truncator &operator=(const background_truncator &) = delete;
+
+ ~background_truncator()
+ {
+ stop.store(true, std::memory_order_relaxed);
+
+ if (thr.joinable()) {
+ thr.join();
+ }
+
+ if (fd != -1) {
+ close(fd);
+ }
+ }
+
+ bool valid() const
+ {
+ return fd != -1;
+ }
+
+ unsigned long toggle_count() const
+ {
+ return toggles.load(std::memory_order_relaxed);
+ }
+
+private:
+ std::thread thr;
+ std::atomic<bool> stop{false};
+ std::atomic<unsigned long> toggles{0};
+ int fd = -1;
+};
+
/*
* Owns the pool that the snapshots are allocated from plus the last GError,
* so that neither leaks when an assertion fails.
CHECK(segment_payload(seg) == expected);
}
+ TEST_CASE("a concurrently truncated object is survived rather than faulted on")
+ {
+ /*
+ * The test above resizes the object only *after* the call has returned,
+ * so it never touches the interval that actually hurts: the one between
+ * the fstat that validates the request and the copy that fulfils it.
+ * A client owns the object and may truncate it exactly there, and a
+ * copy out of a mapping then reads pages that no longer exist, which is
+ * SIGBUS and a dead worker rather than a failed request. Reading the
+ * window instead cannot fault; it merely returns fewer bytes.
+ *
+ * Which of the two permitted outcomes a given iteration gets is up to
+ * the scheduler, so only the outcomes themselves are asserted: either a
+ * snapshot no longer than the window that was asked for, or a refusal
+ * that says why. Completing the loop at all is the regression test.
+ */
+ constexpr gsize big_size = 2 * 1024 * 1024;
+ constexpr gsize small_size = 4096;
+ constexpr gsize win_len = 512 * 1024;
+ /* At the very end, so that shrinking really does remove its pages */
+ constexpr gsize win_off = big_size - win_len;
+ constexpr int iterations = 600;
+
+ backing_object obj("truncate_race", big_size);
+ REQUIRE(obj.valid());
+
+ /*
+ * Zero filled on purpose: ftruncate only ever zero fills, so whatever
+ * the two threads do, every byte that is really read back is a zero.
+ * A data_len that reported the requested length rather than the number
+ * of bytes that were actually read would therefore hand out
+ * uninitialised pool memory, and that is visible right here.
+ */
+ REQUIRE(obj.fill_with('\0'));
+
+ background_truncator truncator(obj, big_size, small_size);
+ REQUIRE(truncator.valid());
+
+ const std::string zeros(win_len, '\0');
+ const auto off_str = std::to_string(win_off);
+ const auto len_str = std::to_string(win_len);
+
+ int completed = 0, snapshots = 0, refusals = 0;
+ int short_reads = 0, partial_reads = 0;
+ int oversized = 0, unexplained = 0, retained = 0, dirty = 0;
+ int null_data = 0;
+
+ for (int i = 0; i < iterations; i++) {
+ /* One pool per iteration, so that the snapshots cannot pile up */
+ segment_mapper mapper;
+ auto *seg = mapper.map(obj.name(), off_str.c_str(), len_str.c_str());
+
+ completed++;
+
+ if (seg == nullptr) {
+ /* Losing the race is fine as long as the refusal says why */
+ refusals++;
+
+ if (mapper.error() == nullptr) {
+ unexplained++;
+ }
+
+ continue;
+ }
+
+ snapshots++;
+
+ if (seg->data == nullptr) {
+ /* Even an empty snapshot is copied from by the callers */
+ null_data++;
+ continue;
+ }
+
+ if (seg->data_len > win_len) {
+ oversized++;
+ continue;
+ }
+
+ if (seg->data_len < win_len) {
+ short_reads++;
+
+ if (seg->data_len > 0) {
+ /* The object shrank in the middle of the read itself */
+ partial_reads++;
+ }
+ }
+
+ if (seg->map != nullptr || seg->map_len != 0 || seg->fd != -1) {
+ retained++;
+ }
+
+ /*
+ * Reads every byte that was reported as present, so that a
+ * data_len covering bytes that were never read shows up as the
+ * fill pattern of a fresh allocation, and one running past the
+ * allocation altogether is caught by the sanitiser
+ */
+ if (memcmp(seg->data, zeros.data(), seg->data_len) != 0) {
+ dirty++;
+ }
+ }
+
+ INFO("snapshots: " << snapshots << ", refusals: " << refusals
+ << ", short reads: " << short_reads
+ << " (" << partial_reads << " partial)"
+ << ", toggles: " << truncator.toggle_count());
+
+ /* Getting this far at all is the point: the old reader took SIGBUS */
+ CHECK(completed == iterations);
+ CHECK(snapshots + refusals == iterations);
+ /* Otherwise nothing was ever raced and the loop proves nothing */
+ CHECK(snapshots > 0);
+ CHECK(null_data == 0);
+ /* Never more than was asked for, whatever the object did meanwhile */
+ CHECK(oversized == 0);
+ CHECK(unexplained == 0);
+ /* The read path keeps neither a mapping nor a descriptor */
+ CHECK(retained == 0);
+ /* data_len bounds real content, not the tail of a fresh allocation */
+ CHECK(dirty == 0);
+ }
+
TEST_CASE("rspamd_task_allow_file_shm_input")
{
CHECK(rspamd_task_allow_file_shm_input(nullptr) == FALSE);
projects / thirdparty / rspamd.git / commitdiff
