Skip to content

Commit 8758b51

Browse files
committed
dofs: Guard the staged-chunk link path
linkStagedChunksSync writes chunk rows from a list it did not produce, so the checks writeFile used to run on the way into the sync apply path have to live in the helper itself. Positional reads find the chunk covering an offset by dividing that offset by CHUNK_SIZE, and take a chunk's start offset to be its index times CHUNK_SIZE. A local writer chunks with chunksOf and satisfies that by construction, but a chunk list that arrived over the wire only satisfies it while both sides window at the same size. Reject a list whose interior chunks are short or whose chunks overflow a window, so a sender that windows differently fails loudly instead of producing a file whose bytes read back from the wrong offsets. Also restore the read-only mount check. applyChanges gates read-only mount roots itself and reports the skipped entries, so behavior over sync is unchanged, but the exported helper is a write primitive and the guard belongs at the data layer. Run both checks in applyFileEntry before the existing entry at the path is removed. A batch is a sequence of independent transactions, so a throw after the removal would drop a file and put nothing in its place.
1 parent b96015e commit 8758b51

5 files changed

Lines changed: 168 additions & 5 deletions

File tree

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"@cloudflare/dofs": patch
3+
"@cloudflare/computer": patch
4+
---
5+
6+
Cut peak memory during a sync pull. Applying a file entry now links the chunks the sender already staged instead of reading them back and joining them into one whole-file buffer, which used to hold roughly twice the file size in the isolate at once.

packages/dofs/src/fs/mount-guard.test.ts

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
1+
import { createHash } from "node:crypto";
2+
13
import { describe, expect, it } from "vitest";
24

35
import type { Database } from "../storage.js";
6+
import { stageBlob } from "../sync/blobs.js";
47
import { mkdir } from "./mkdir.js";
58
import {
69
assertNotReadOnly,
@@ -12,7 +15,7 @@ import { resolveInode } from "./resolve.js";
1215
import { rm } from "./rm.js";
1316
import { symlink } from "./symlink.js";
1417
import { withDB } from "./with-db.js";
15-
import { writeFile, writeFileSync } from "./writeFile.js";
18+
import { linkStagedChunksSync, writeFile, writeFileSync } from "./writeFile.js";
1619

1720
// Stage a read-only mount the way the workspace-side indexer
1821
// eventually will: a row in `_vfs_mounts` plus an actual subtree
@@ -141,6 +144,29 @@ describe("writeFile under a read-only mount", () => {
141144
});
142145
});
143146

147+
it("rejects linkStagedChunksSync under the mount root with EROFS", async () => {
148+
await withDB(async (db) => {
149+
mkdir(db, "/workspace/r2", { recursive: true }, () => 0);
150+
stageMount(db, "/workspace/r2", "read-only");
151+
152+
const bytes = new TextEncoder().encode("blocked");
153+
const hash = new Uint8Array(createHash("sha256").update(bytes).digest());
154+
stageBlob(db, hash, bytes, 0);
155+
156+
expect(() =>
157+
linkStagedChunksSync(
158+
db,
159+
"/workspace/r2/hello.txt",
160+
["workspace", "r2", "hello.txt"],
161+
[{ hash, size: bytes.byteLength }],
162+
{},
163+
0,
164+
),
165+
).toThrow(/EROFS|read-only/);
166+
expect(resolveInode(db, "/workspace/r2/hello.txt")).toBeNull();
167+
});
168+
});
169+
144170
it("allows writes under a read-write mount", async () => {
145171
await withDB(async (db) => {
146172
mkdir(db, "/workspace/rw", { recursive: true }, () => 0);

packages/dofs/src/fs/writeFile.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -214,8 +214,33 @@ async function writeFileStreaming(
214214
linkStagedChunksSync(db, canonical, parts, chunkRefs, { ...options, mode }, mtime);
215215
}
216216

217+
// Reject a chunk list that positional reads could not address.
218+
// readRangeSync finds the chunk covering an offset by dividing that
219+
// offset by CHUNK_SIZE, and takes a chunk's start offset to be its
220+
// index times CHUNK_SIZE, so every chunk but the last has to fill a
221+
// whole window and none may overflow one. Local writers chunk with
222+
// chunksOf and satisfy this by construction; a chunk list that
223+
// arrived over sync does not have to.
224+
export function assertChunkWindows(chunkRefs: ChunkRef[], canonical: string): void {
225+
for (let idx = 0; idx < chunkRefs.length; idx++) {
226+
const { size } = chunkRefs[idx];
227+
const last = idx === chunkRefs.length - 1;
228+
if (size === CHUNK_SIZE || (last && size < CHUNK_SIZE)) continue;
229+
throw createWorkspaceError(
230+
"EINVAL",
231+
`chunk ${idx} of ${chunkRefs.length} holds ${size} bytes; only the last chunk may be shorter than ${CHUNK_SIZE}: ${canonical}`,
232+
canonical,
233+
);
234+
}
235+
}
236+
217237
// Link a path to chunks already staged in content-addressed storage.
218238
// This keeps payload bytes out of memory during sync apply.
239+
//
240+
// The chunk list comes from a caller that did its own chunking, so
241+
// the guards every other write path gets from writeFile have to run
242+
// here too: the read-only mount check, and the fixed-window layout
243+
// that positional reads depend on.
219244
export function linkStagedChunksSync(
220245
db: Database,
221246
canonical: string,
@@ -224,6 +249,8 @@ export function linkStagedChunksSync(
224249
options: WriteFileOptions,
225250
mtime: number,
226251
): void {
252+
assertNotReadOnly(db, canonical);
253+
assertChunkWindows(chunkRefs, canonical);
227254
const mode = (options.mode ?? 0o644) & 0o7777;
228255
db.transactionSync(() => {
229256
const parentInode = resolveParent(db, parts, canonical);

packages/dofs/src/sync/apply.test.ts

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { createHash } from "node:crypto";
2+
13
import { describe, expect, it, vi } from "vitest";
24

35
import { link } from "../fs/link.js";
@@ -142,6 +144,103 @@ describe("applyChanges", () => {
142144
);
143145
});
144146

147+
it("rejects a file entry whose interior chunks are not chunk-aligned", async () => {
148+
// Positional reads locate a chunk by dividing the offset by
149+
// CHUNK_SIZE, so only the final chunk may be short. Linking a
150+
// sender's chunk list verbatim has to enforce that.
151+
await withDB(async (db) => {
152+
const first = new TextEncoder().encode("first");
153+
const second = new TextEncoder().encode("second");
154+
const chunks = [first, second].map((bytes) => {
155+
const hash = new Uint8Array(createHash("sha256").update(bytes).digest());
156+
stageBlob(db, hash, bytes, 1000);
157+
return { hash, size: bytes.byteLength };
158+
});
159+
160+
await expect(
161+
applyChanges(
162+
db,
163+
[
164+
{
165+
kind: "file",
166+
rev: 1,
167+
path: "/ragged.txt",
168+
mode: 0o644,
169+
mtime: 1000,
170+
size: first.byteLength + second.byteLength,
171+
chunks,
172+
},
173+
],
174+
new Map(),
175+
{ source: "upstream" },
176+
),
177+
).rejects.toThrow(/chunk/);
178+
expect(resolveInode(db, "/ragged.txt")).toBeNull();
179+
});
180+
});
181+
182+
it("leaves the existing file in place when it rejects a ragged entry", async () => {
183+
await withDB(async (db) => {
184+
await writeFile(db, "/keep.txt", "original", {}, () => 1000);
185+
186+
const first = new TextEncoder().encode("first");
187+
const second = new TextEncoder().encode("second");
188+
const chunks = [first, second].map((bytes) => {
189+
const hash = new Uint8Array(createHash("sha256").update(bytes).digest());
190+
stageBlob(db, hash, bytes, 1000);
191+
return { hash, size: bytes.byteLength };
192+
});
193+
194+
await expect(
195+
applyChanges(
196+
db,
197+
[
198+
{
199+
kind: "file",
200+
rev: 2,
201+
path: "/keep.txt",
202+
mode: 0o644,
203+
mtime: 2000,
204+
size: first.byteLength + second.byteLength,
205+
chunks,
206+
},
207+
],
208+
new Map(),
209+
{ source: "upstream" },
210+
),
211+
).rejects.toThrow(/chunk/);
212+
expect(await readFile(db, "/keep.txt", "utf8")).toBe("original");
213+
});
214+
});
215+
216+
it("rejects a file entry with a chunk larger than the chunk size", async () => {
217+
await withDB(async (db) => {
218+
const bytes = new Uint8Array(CHUNK_SIZE + 1);
219+
const hash = new Uint8Array(createHash("sha256").update(bytes).digest());
220+
stageBlob(db, hash, bytes, 1000);
221+
222+
await expect(
223+
applyChanges(
224+
db,
225+
[
226+
{
227+
kind: "file",
228+
rev: 1,
229+
path: "/oversized.bin",
230+
mode: 0o644,
231+
mtime: 1000,
232+
size: bytes.byteLength,
233+
chunks: [{ hash, size: bytes.byteLength }],
234+
},
235+
],
236+
new Map(),
237+
{ source: "upstream" },
238+
),
239+
).rejects.toThrow(/chunk/);
240+
expect(resolveInode(db, "/oversized.bin")).toBeNull();
241+
});
242+
});
243+
145244
it("commits in batches capped by byte budget", async () => {
146245
// Force many small files; with a tiny byte budget the apply
147246
// path should still converge, just across more batches. We

packages/dofs/src/sync/apply.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import { invalidateResolveSubtree } from "../fs/resolveCache.js";
55
import { rm } from "../fs/rm.js";
66
import { symlink } from "../fs/symlink.js";
77
import { unlinkDirent } from "../fs/unlink.js";
8-
import { linkStagedChunksSync } from "../fs/writeFile.js";
8+
import { assertChunkWindows, linkStagedChunksSync } from "../fs/writeFile.js";
99
import { canonicalizePath } from "../path.js";
1010
import { incrementRev } from "../rev.js";
1111
import type { Database } from "../storage.js";
@@ -397,15 +397,20 @@ export function applyChangesSync(
397397
}
398398

399399
// Link a file entry to staged chunks without loading payload bytes.
400-
// In-memory objects are staged individually before the link.
401-
402-
// Validate declared sizes without loading payloads; chunk hashes remain
400+
// In-memory objects are staged individually before the link. Declared
401+
// sizes are validated without loading payloads; chunk hashes remain
403402
// trusted here, matching stageBlob's existing contract.
403+
//
404+
// Every check runs before removeReplaceableFinalEntry, so a rejected
405+
// entry leaves whatever was already at the path alone. Batches are a
406+
// sequence of independent transactions, so a throw part way through
407+
// does not roll the removal back.
404408
function applyFileEntry(
405409
db: Database,
406410
entry: Extract<ChangeEntry, { kind: "file" }>,
407411
objects: Map<string, Uint8Array>,
408412
): number {
413+
assertChunkWindows(entry.chunks, entry.path);
409414
let total = 0;
410415
for (const c of entry.chunks) {
411416
total += c.size;

0 commit comments

Comments
 (0)