[PR #385] [MERGED] Document unsafe pointer lifetime constraints in update_chunk() #406

New issue

Closed

opened 2026-10-26 09:49:58 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 09:49:58 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/deepseek-ai/3FS/pull/385
Author: @Copilot
Created: 1/20/2026
Status: ✅ Merged
Merged: 1/20/2026
Merged by: @SF-Zhou

Base: main ← Head: copilot/fix-memory-safety-update-chunk

📝 Commits (3)

574bdbb Initial plan
7e3436d Add SAFETY comments to document unsafe pointer usage in update_chunk()
9f0be1e Fix line numbers in SAFETY comment to reflect actual code locations

📊 Changes

2 files changed (+7 additions, -0 deletions)

View changed files

📝 src/storage/chunk_engine/src/core/engine.rs (+4 -0)
📝 src/storage/chunk_engine/src/cxx.rs (+3 -0)

📄 Description

The unsafe { std::slice::from_raw_parts(...) } in update_chunk() relies on synchronous data consumption—a constraint that would become a use-after-free hazard if writes ever become asynchronous.

Changes

src/storage/chunk_engine/src/core/engine.rs:298: Added SAFETY comment documenting that req.data pointer must remain valid for function duration, consumed synchronously in safe_write/copy_on_write calls at lines 386-394, 398-404, 418-424
src/storage/chunk_engine/src/cxx.rs:381: Added SAFETY comment documenting that data: u64 field represents a raw pointer valid for update_chunk() duration

// SAFETY: req.data pointer must be valid and remain live for the duration of this function.
// The slice is consumed synchronously and immediately within safe_write/copy_on_write calls
// (lines 386-394, 398-404, 418-424). If writes become asynchronous in the future,
// this would require refactoring to take ownership of the data (e.g., via Vec<u8>).
let data = unsafe { std::slice::from_raw_parts(req.data as *const _, req.length as usize) };

Original prompt

This section details on the original issue you should resolve

<issue_title>[SAFETY] Memory safety consideration in update_chunk()</issue_title>
<issue_description>[SAFETY] Memory safety consideration in update_chunk()

Hi team,

I was studying the Engine module to understand its architecture and noticed a subtle safety constraint in update_chunk().

Observation:
The unsafe { std::slice::from_raw_parts(...) } in update_chunk() assumes the req.data pointer remains valid only because the slice is consumed immediately within safe_write/copy_on_write.

Why this matters now:
This is perfectly safe in the current synchronous flow. However, if we ever optimize by making writes asynchronous (write-behind, background flush), this would become a use-after-free hazard.

Low-effort safeguard suggestion:

Add a // SAFETY: comment above the unsafe block documenting this synchronous consumption requirement

(Future) Consider providing a safe API layer that accepts &[u8] alongside the raw pointer interface

This isn't a pressing bug, but documenting this constraint will prevent accidental memory safety issues during future optimizations.

Thoughts?</issue_description>

<agent_instructions>Add a // SAFETY comment to https://github.com/deepseek-ai/3FS/blob/main/src/storage/chunk_engine/src/core/engine.rs#L299 and https://github.com/deepseek-ai/3FS/blob/main/src/storage/chunk_engine/src/cxx.rs#L381</agent_instructions>

Comments on the Issue (you are @copilot in this section)
@SF-Zhou Thanks for catching this — I'll add a SAFETY comment to document the assumption.

Fixes deepseek-ai/3FS#384

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/deepseek-ai/3FS/pull/385 **Author:** [@Copilot](https://github.com/apps/copilot-swe-agent) **Created:** 1/20/2026 **Status:** ✅ Merged **Merged:** 1/20/2026 **Merged by:** [@SF-Zhou](https://github.com/SF-Zhou) **Base:** `main` ← **Head:** `copilot/fix-memory-safety-update-chunk` --- ### 📝 Commits (3) - [`574bdbb`](https://github.com/deepseek-ai/3FS/commit/574bdbb22f9346f0b9e1ba9973b6345cf07ec611) Initial plan - [`7e3436d`](https://github.com/deepseek-ai/3FS/commit/7e3436df401de26d46636a42a556c4cd05288b0f) Add SAFETY comments to document unsafe pointer usage in update_chunk() - [`9f0be1e`](https://github.com/deepseek-ai/3FS/commit/9f0be1e3e6ce74349033e411b32cadbd6b63b568) Fix line numbers in SAFETY comment to reflect actual code locations ### 📊 Changes **2 files changed** (+7 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `src/storage/chunk_engine/src/core/engine.rs` (+4 -0) 📝 `src/storage/chunk_engine/src/cxx.rs` (+3 -0) </details> ### 📄 Description The `unsafe { std::slice::from_raw_parts(...) }` in `update_chunk()` relies on synchronous data consumption—a constraint that would become a use-after-free hazard if writes ever become asynchronous. ## Changes - **`src/storage/chunk_engine/src/core/engine.rs:298`**: Added `SAFETY` comment documenting that `req.data` pointer must remain valid for function duration, consumed synchronously in `safe_write`/`copy_on_write` calls at lines 386-394, 398-404, 418-424 - **`src/storage/chunk_engine/src/cxx.rs:381`**: Added `SAFETY` comment documenting that `data: u64` field represents a raw pointer valid for `update_chunk()` duration ```rust // SAFETY: req.data pointer must be valid and remain live for the duration of this function. // The slice is consumed synchronously and immediately within safe_write/copy_on_write calls // (lines 386-394, 398-404, 418-424). If writes become asynchronous in the future, // this would require refactoring to take ownership of the data (e.g., via Vec<u8>). let data = unsafe { std::slice::from_raw_parts(req.data as *const _, req.length as usize) }; ```  <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>[SAFETY] Memory safety consideration in update_chunk()</issue_title> > <issue_description>[SAFETY] Memory safety consideration in update_chunk() > > Hi team, > > I was studying the Engine module to understand its architecture and noticed a subtle safety constraint in `update_chunk()`. > > **Observation:** > The `unsafe { std::slice::from_raw_parts(...) }` in `update_chunk()` assumes the `req.data` pointer remains valid only because the slice is consumed immediately within `safe_write`/`copy_on_write`. > > **Why this matters now:** > This is perfectly safe in the current synchronous flow. However, if we ever optimize by making writes asynchronous (write-behind, background flush), this would become a use-after-free hazard. > > **Low-effort safeguard suggestion:** > 1. Add a `// SAFETY:` comment above the unsafe block documenting this synchronous consumption requirement > 2. (Future) Consider providing a safe API layer that accepts `&[u8]` alongside the raw pointer interface > > This isn't a pressing bug, but documenting this constraint will prevent accidental memory safety issues during future optimizations. > > Thoughts?</issue_description> > > <agent_instructions>Add a `// SAFETY` comment to https://github.com/deepseek-ai/3FS/blob/main/src/storage/chunk_engine/src/core/engine.rs#L299 and https://github.com/deepseek-ai/3FS/blob/main/src/storage/chunk_engine/src/cxx.rs#L381</agent_instructions> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@SF-Zhou</author><body> > Thanks for catching this — I'll add a SAFETY comment to document the assumption.</body></comment_new> > </comments> > </details>  - Fixes deepseek-ai/3FS#384  --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 09:49:58 +00:00