Fix crash in FileBufferedChannel

Author: CamJN

CHANGELOG

@@ -12,6 +12,7 @@ Release 6.2.1 (Not yet released)
  * Fix reporting of application output printed during smart spawning preloader startup.
  * Include captured subprocess output in application spawn failure messages in logs, allowing easier troubleshooting without reading the HTML report.
  * `passenger-config reopen-logs` now reopens both stdout and stderr, and can exit successfully when Passenger is not running by passing `--ignore-passenger-not-running`. Closes GH-2259.
+ * Fix an occasional Passenger core crash after serving a large response that Passenger generates itself, such as an application error page. Requests arriving while the core restarted failed with 502 Bad Gateway.
 
 
 Release 6.2.0

src/cxx_supportlib/ServerKit/FileBufferedChannel.h

@@ -486,6 +486,53 @@ class FileBufferedChannel: protected Channel {
 		FBC_DEBUG("pushBuffer() completed: nbuffers = " << nbuffers << ", bytesBuffered = " << bytesBuffered);
 	}
 
+	/**
+	 * Like `pushBuffer()`, but if `buffer` merely wraps memory owned by the caller
+	 * (i.e. it has no mbuf_block that keeps its memory alive), then the data is first
+	 * copied into mbufs from the pool. Large buffers are split over multiple mbufs.
+	 *
+	 * Must be used for all buffers queued in the in-file mode. There, buffers stay
+	 * queued until the writer has moved them to the file, which may be long after the
+	 * reader has passed them to the underlying channel. Callers (e.g. HttpServer
+	 * writing a response body from the request's memory pool) only guarantee that
+	 * such memory stays valid until the reader has passed it on.
+	 */
+	void pushBufferWithOwnedMemory(const MemoryKit::mbuf &buffer) {
+		if (buffer.mbuf_block != NULL || buffer.empty()) {
+			pushBuffer(buffer);
+			return;
+		}
+
+		size_t chunkSize = MemoryKit::mbuf_pool_data_size(&ctx->mbuf_pool);
+		const char *pos = buffer.start;
+		while (pos < buffer.end) {
+			size_t size = std::min<size_t>(buffer.end - pos, chunkSize);
+			MemoryKit::mbuf copy = MemoryKit::mbuf_get(&ctx->mbuf_pool);
+			memcpy(copy.start, pos, size);
+			pushBuffer(MemoryKit::mbuf(copy, 0, size));
+			pos += size;
+		}
+	}
+
+	/**
+	 * Called when switching to the in-file mode: ensures that buffers that were
+	 * queued during the in-memory mode don't reference caller-owned memory either.
+	 * See `pushBufferWithOwnedMemory()` for why.
+	 */
+	void takeOwnershipOfQueuedBuffers() {
+		deque<MemoryKit::mbuf> buffers;
+		if (nbuffers > 0) {
+			buffers.push_back(firstBuffer);
+			buffers.insert(buffers.end(), moreBuffers.begin(), moreBuffers.end());
+		}
+		clearBuffers(false);
+
+		deque<MemoryKit::mbuf>::const_iterator it, end = buffers.end();
+		for (it = buffers.begin(); it != end; it++) {
+			pushBufferWithOwnedMemory(*it);
+		}
+	}
+
 	void popBuffer() {
 		assert(bytesBuffered >= firstBuffer.size());
 		bytesBuffered -= firstBuffer.size();
@@ -836,6 +883,7 @@ class FileBufferedChannel: protected Channel {
 		P_ASSERT_EQ(inFileMode, 0);
 
 		FBC_DEBUG("Switching to in-file mode");
+		takeOwnershipOfQueuedBuffers();
 		mode = IN_FILE_MODE;
 		inFileMode = boost::make_shared<InFileMode>(ctx->libuv);
 		createBufferFile();
@@ -1423,7 +1471,11 @@ class FileBufferedChannel: protected Channel {
 			FBC_DEBUG("Feeding aborted: EOF or error detected");
 			return;
 		}
-		pushBuffer(buffer);
+		if (mode == IN_FILE_MODE) {
+			pushBufferWithOwnedMemory(buffer);
+		} else {
+			pushBuffer(buffer);
+		}
 		if (mode == IN_MEMORY_MODE && passedThreshold()) {
 			switchToInFileMode();
 		} else if (mode == IN_FILE_MODE

test/cxx/ServerKit/FileBufferedChannelTest.cpp

@@ -220,6 +220,23 @@ namespace tut {
 		void _setChannelDataCallback(FileBufferedChannel::DataCallback callback) {
 			channel.setDataCallback(callback);
 		}
+
+		// Feeds a buffer that merely wraps caller-owned memory, like HttpServer does
+		// with response bodies allocated from a request's memory pool. Once the reader
+		// has passed the data on, the memory is freed, like HttpServer does when it
+		// resets the pool at the end of the request.
+		void feedCallerOwnedDataAndFreeItOncePassedOn(const string &data) {
+			bg.safe->runSync(boost::bind(
+				&ServerKit_FileBufferedChannelTest::_feedCallerOwnedDataAndFreeItOncePassedOn,
+				this, data));
+		}
+
+		void _feedCallerOwnedDataAndFreeItOncePassedOn(string data) {
+			char *memory = new char[data.size()];
+			memcpy(memory, data.data(), data.size());
+			channel.feed(memory, data.size());
+			delete[] memory;
+		}
 	};
 
 	DEFINE_TEST_GROUP_WITH_LIMIT(ServerKit_FileBufferedChannelTest, 100);
@@ -1255,4 +1272,41 @@ namespace tut {
 			result = log.find("Error: ") == 0;
 		);
 	}
+
+	TEST_METHOD(51) {
+		set_test_name("In the in-file mode, buffers that wrap caller-owned memory are "
+			"still moved to the file after the caller freed that memory, so the "
+			"writer must not read from the caller's memory");
+
+		// Detecting the use-after-free requires AddressSanitizer or Valgrind,
+		// which CI uses for the C++ tests.
+		//
+		// This is deterministic: the buffer file is created in the background, but
+		// its completion is processed on the event loop, which is busy with
+		// feeding and freeing until the data has been freed. Only then can the
+		// writer start moving the buffer to the file.
+		Json::Value config;
+		vector<ConfigKit::Error> errors;
+		config["file_buffered_channel_threshold"] = 1;
+		// Prevent the reader from discarding the queued buffer (by switching back
+		// to the in-memory mode) once it has passed all data on.
+		config["file_buffered_channel_auto_truncate_file"] = false;
+		ensure(context.configure(config, errors));
+
+		startLoop();
+
+		feedCallerOwnedDataAndFreeItOncePassedOn("hello");
+		{
+			// The data was passed on before it was freed.
+			LOCK();
+			ensure_equals(log, "Data: hello\n");
+		}
+		ensure_equals(getChannelMode(), FileBufferedChannel::IN_FILE_MODE);
+
+		// Wait until the writer has moved the buffer to the file.
+		EVENTUALLY(5,
+			result = getChannelWriterState() == FileBufferedChannel::WS_INACTIVE
+				&& getChannelBytesBuffered() == 0;
+		);
+	}
 }