From c2d813f3ec755765df7983a77617c115b6fc3dbb Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Wed, 11 Sep 2024 13:24:54 +0200 Subject: [PATCH] reduce max comment depth to 50 --- Cargo.lock | 3 +-- Cargo.toml | 2 +- api_tests/src/comment.spec.ts | 4 +--- crates/api_crud/src/comment/create.rs | 4 +--- crates/apub/src/protocol/objects/note.rs | 17 ++++++++++++++--- crates/utils/src/lib.rs | 2 ++ 6 files changed, 20 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5dcdd045c..a41b48b94 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -17,8 +17,7 @@ checksum = "8f27d075294830fcab6f66e320dab524bc6d048f4a151698e153205559113772" [[package]] name = "activitypub_federation" version = "0.5.8" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b86eea7a032da501fe07b04a83c10716ea732c45e6943d7f045bc053aca03f2a" +source = "git+https://github.com/LemmyNet/activitypub-federation-rust.git?branch=nested-comments-stack-overflow#4b94af9ea0580226aaa10a4abb004d0b8ca10eae" dependencies = [ "activitystreams-kinds", "actix-web", diff --git a/Cargo.toml b/Cargo.toml index 38d4adc5f..5c74e7cad 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -100,7 +100,7 @@ lemmy_db_views = { version = "=0.19.6-beta.7", path = "./crates/db_views" } lemmy_db_views_actor = { version = "=0.19.6-beta.7", path = "./crates/db_views_actor" } lemmy_db_views_moderator = { version = "=0.19.6-beta.7", path = "./crates/db_views_moderator" } lemmy_federate = { version = "=0.19.6-beta.7", path = "./crates/federate" } -activitypub_federation = { version = "0.5.8", default-features = false, features = [ +activitypub_federation = { git = "https://github.com/LemmyNet/activitypub-federation-rust.git", branch = "nested-comments-stack-overflow", default-features = false, features = [ "actix-web", ] } diesel = "2.1.6" diff --git a/api_tests/src/comment.spec.ts b/api_tests/src/comment.spec.ts index 77b9c3a20..54c3c8bc4 100644 --- a/api_tests/src/comment.spec.ts +++ b/api_tests/src/comment.spec.ts @@ -860,9 +860,7 @@ test("Dont send a comment reply to a blocked community", async () => { }); /// Fetching a deeply nested comment can lead to stack overflow as all parent comments are also -/// fetched recursively. Ensure that it works properly. Stack size depends on the OS so this -/// may fail in some cases. In release builds Lemmy is more optimized and can handle the max depth -/// of 100. +/// fetched recursively. Ensure that it works properly. test("Fetch a deeply nested comment", async () => { let lastComment; for (let i = 0; i < 50; i++) { diff --git a/crates/api_crud/src/comment/create.rs b/crates/api_crud/src/comment/create.rs index 49de9d5de..b6bd2ccc3 100644 --- a/crates/api_crud/src/comment/create.rs +++ b/crates/api_crud/src/comment/create.rs @@ -29,11 +29,9 @@ use lemmy_db_schema::{ use lemmy_db_views::structs::{LocalUserView, PostView}; use lemmy_utils::{ error::{LemmyErrorExt, LemmyErrorType, LemmyResult}, - utils::{mention::scrape_text_for_mentions, validation::is_valid_body_field}, + utils::{mention::scrape_text_for_mentions, validation::is_valid_body_field}, MAX_COMMENT_DEPTH_LIMIT, }; -const MAX_COMMENT_DEPTH_LIMIT: usize = 100; - #[tracing::instrument(skip(context))] pub async fn create_comment( data: Json, diff --git a/crates/apub/src/protocol/objects/note.rs b/crates/apub/src/protocol/objects/note.rs index 3be80e481..f823b0496 100644 --- a/crates/apub/src/protocol/objects/note.rs +++ b/crates/apub/src/protocol/objects/note.rs @@ -20,7 +20,7 @@ use lemmy_db_schema::{ source::{community::Community, post::Post}, traits::Crud, }; -use lemmy_utils::{error::LemmyResult, LemmyErrorType}; +use lemmy_utils::{error::{LemmyError, LemmyResult}, LemmyErrorType, MAX_COMMENT_DEPTH_LIMIT}; use serde::{Deserialize, Serialize}; use serde_with::skip_serializing_none; use url::Url; @@ -57,13 +57,24 @@ impl Note { &self, context: &Data, ) -> LemmyResult<(ApubPost, Option)> { - // Fetch parent comment chain in a box, otherwise it can cause a stack overflow. + // We use recursion here to fetch the entire comment chain up to the top-level parent. This is + // necessary because we need to know the post and parent comment in order to insert a new + // comment. However it can also lead to stack overflow when fetching many comments recursively. + // To avoid this we check the request count against max comment depth, which based on testing + // can be handled without risking stack overflow. This is not a perfect solution, because in + // some cases we have to fetch user profiles too, and reach the limit after only 25 comments + // or so. + // A cleaner solution would be converting the recursion into a loop, but that is tricky. + // Use a lower request limit here. Otherwise we can run into stack overflow due to recursion. + if context.request_count() > MAX_COMMENT_DEPTH_LIMIT as u32 { + Err(LemmyErrorType::MaxCommentDepthReached)?; + } let parent = self.in_reply_to.dereference(context).await?; match parent { PostOrComment::Post(p) => Ok((p.clone(), None)), PostOrComment::Comment(c) => { let post_id = c.post_id; - let post = Post::read(&mut context.pool(), post_id) + let post = Box::pin(Post::read(&mut context.pool(), post_id)) .await? .ok_or(LemmyErrorType::CouldntFindPost)?; Ok((post.into(), Some(c.clone()))) diff --git a/crates/utils/src/lib.rs b/crates/utils/src/lib.rs index 5a5e76d2a..7f0691496 100644 --- a/crates/utils/src/lib.rs +++ b/crates/utils/src/lib.rs @@ -29,6 +29,8 @@ pub const CACHE_DURATION_FEDERATION: Duration = Duration::from_secs(60); pub const CACHE_DURATION_API: Duration = Duration::from_secs(1); +pub const MAX_COMMENT_DEPTH_LIMIT: usize = 50; + #[macro_export] macro_rules! location_info { () => {