From 8bff3b321329647b7b575a96743373e70ac3692d Mon Sep 17 00:00:00 2001 From: 7y-9 Date: Thu, 16 Jul 2026 14:24:04 +0800 Subject: [PATCH] fix(volume): reject overflowing needle ID deltas (#10342) * fix: reject overflowing needle ID deltas Problem: Parsing a file ID with a delta can wrap a valid maximum needle ID back to zero without returning an error. Root cause: Needle.ParsePath added the parsed uint64 delta without checking whether the sum exceeded the needle ID range. Fix: Compare the delta with the remaining uint64 capacity before addition and return a contextual overflow error when it does not fit. Validation: go test ./weed/storage/needle -run ^TestNeedleParsePathRejectsDeltaOverflow$ -count=1; go test ./weed/storage/needle -count=1; git diff --check 10cdaf381875492a2c752d1038797e96ff18208f..HEAD Co-authored-by: Codex * fix: propagate needle ID delta parse errors Co-authored-by: Codex * print the needle id in hex in the delta overflow error * batch delete: keep processing after a cookie mismatch * rust volume: reject overflowing needle id deltas --------- Co-authored-by: Codex Co-authored-by: Chris Lu --- seaweed-volume/src/storage/needle/needle.rs | 15 ++++++- weed/server/volume_grpc_batch_delete.go | 15 +++++-- weed/server/volume_grpc_query.go | 5 ++- weed/server/volume_server_handlers_write.go | 5 ++- weed/server/volume_server_parse_path_test.go | 45 ++++++++++++++++++++ weed/storage/needle/needle.go | 4 ++ weed/storage/needle/needle_test.go | 8 ++++ 7 files changed, 90 insertions(+), 7 deletions(-) create mode 100644 weed/server/volume_server_parse_path_test.go diff --git a/seaweed-volume/src/storage/needle/needle.rs b/seaweed-volume/src/storage/needle/needle.rs index 7df518e9d..81384d47b 100644 --- a/seaweed-volume/src/storage/needle/needle.rs +++ b/seaweed-volume/src/storage/needle/needle.rs @@ -712,7 +712,7 @@ fn format_needle_id_cookie(key: NeedleId, cookie: Cookie) -> String { /// Parse "needle_id_cookie_hex" or "needle_id_cookie_hex_delta" into (NeedleId, Cookie). /// Matches Go's ParsePath + ParseNeedleIdCookie: supports an optional `_delta` suffix /// where delta is a decimal number added to the NeedleId (used for sub-file addressing). -/// Rejects strings that are too short or too long. +/// Rejects strings that are too short or too long, and deltas that overflow the needle id. pub fn parse_needle_id_cookie(s: &str) -> Result<(NeedleId, Cookie), String> { // Go ParsePath: check for "_" suffix containing a decimal delta let (hex_part, delta) = if let Some(underscore_pos) = s.rfind('_') { @@ -754,7 +754,11 @@ pub fn parse_needle_id_cookie(s: &str) -> Result<(NeedleId, Cookie), String> { // Apply delta if present (Go: n.Id += Uint64ToNeedleId(d)) if let Some(d) = delta { - key = NeedleId(key.0.wrapping_add(d)); + key = NeedleId( + key.0 + .checked_add(d) + .ok_or_else(|| format!("delta {} overflows needle id {:x}", d, key.0))?, + ); } Ok((key, cookie)) @@ -937,6 +941,13 @@ mod tests { assert_eq!(cookie, Cookie(0x12345678)); } + #[test] + fn test_parse_rejects_delta_overflow() { + assert!(parse_needle_id_cookie("ffffffffffffffff00000000_1").is_err()); + let (key, _) = parse_needle_id_cookie("fffffffffffffffe00000000_1").unwrap(); + assert_eq!(key, NeedleId(u64::MAX)); + } + #[test] fn test_parse_odd_length_needle_id() { // Go's shell formats purge fids with strconv.FormatUint, which does not diff --git a/weed/server/volume_grpc_batch_delete.go b/weed/server/volume_grpc_batch_delete.go index 1e6d4da71..72d8402ef 100644 --- a/weed/server/volume_grpc_batch_delete.go +++ b/weed/server/volume_grpc_batch_delete.go @@ -35,7 +35,6 @@ func (vs *VolumeServer) BatchDelete(ctx context.Context, req *volume_server_pb.B n := new(needle.Needle) volumeId, _ := needle.NewVolumeId(vid) - ecVolume, isEcVolume := vs.store.FindEcVolume(volumeId) if req.SkipCookieCheck { n.Id, _, err = needle.ParseNeedleIdCookie(id_cookie) if err != nil { @@ -46,7 +45,17 @@ func (vs *VolumeServer) BatchDelete(ctx context.Context, req *volume_server_pb.B continue } } else { - n.ParsePath(id_cookie) + if err := n.ParsePath(id_cookie); err != nil { + resp.Results = append(resp.Results, &volume_server_pb.DeleteResult{ + FileId: fid, + Status: http.StatusBadRequest, + Error: err.Error()}) + continue + } + } + + ecVolume, isEcVolume := vs.store.FindEcVolume(volumeId) + if !req.SkipCookieCheck { cookie := n.Cookie if !isEcVolume { if _, err := vs.store.ReadVolumeNeedle(volumeId, n, nil, nil); err != nil { @@ -73,7 +82,7 @@ func (vs *VolumeServer) BatchDelete(ctx context.Context, req *volume_server_pb.B Status: http.StatusBadRequest, Error: "File Random Cookie does not match.", }) - break + continue } } diff --git a/weed/server/volume_grpc_query.go b/weed/server/volume_grpc_query.go index a1abcb8eb..55fd59790 100644 --- a/weed/server/volume_grpc_query.go +++ b/weed/server/volume_grpc_query.go @@ -21,7 +21,10 @@ func (vs *VolumeServer) Query(req *volume_server_pb.QueryRequest, stream volume_ n := new(needle.Needle) volumeId, _ := needle.NewVolumeId(vid) - n.ParsePath(id_cookie) + if err := n.ParsePath(id_cookie); err != nil { + glog.V(0).Infof("volume query failed to parse fid %s: %v", fid, err) + return err + } cookie := n.Cookie if _, err := vs.store.ReadVolumeNeedle(volumeId, n, nil, nil); err != nil { diff --git a/weed/server/volume_server_handlers_write.go b/weed/server/volume_server_handlers_write.go index c31e3503a..e18fcc6d2 100644 --- a/weed/server/volume_server_handlers_write.go +++ b/weed/server/volume_server_handlers_write.go @@ -83,7 +83,10 @@ func (vs *VolumeServer) DeleteHandler(w http.ResponseWriter, r *http.Request) { n := new(needle.Needle) vid, fid, _, _, _ := parseURLPath(r.URL.Path) volumeId, _ := needle.NewVolumeId(vid) - n.ParsePath(fid) + if err := n.ParsePath(fid); err != nil { + writeJsonError(w, r, http.StatusBadRequest, err) + return + } if !vs.maybeCheckJwtAuthorization(r, vid, fid, true) { writeJsonError(w, r, http.StatusUnauthorized, errors.New("wrong jwt")) diff --git a/weed/server/volume_server_parse_path_test.go b/weed/server/volume_server_parse_path_test.go new file mode 100644 index 000000000..e8fe9a907 --- /dev/null +++ b/weed/server/volume_server_parse_path_test.go @@ -0,0 +1,45 @@ +package weed_server + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/volume_server_pb" +) + +const overflowingNeedleDeltaFid = "1,ffffffffffffffff00000000_1" + +func TestDeleteHandlerRejectsOverflowingNeedleDelta(t *testing.T) { + req := httptest.NewRequest(http.MethodDelete, "/"+overflowingNeedleDeltaFid, nil) + resp := httptest.NewRecorder() + + (&VolumeServer{}).DeleteHandler(resp, req) + + if resp.Code != http.StatusBadRequest { + t.Fatalf("DeleteHandler returned status %d, want %d", resp.Code, http.StatusBadRequest) + } +} + +func TestBatchDeleteRejectsOverflowingNeedleDelta(t *testing.T) { + resp, err := (&VolumeServer{}).BatchDelete(context.Background(), &volume_server_pb.BatchDeleteRequest{ + FileIds: []string{overflowingNeedleDeltaFid}, + }) + if err != nil { + t.Fatalf("BatchDelete returned error: %v", err) + } + if len(resp.Results) != 1 || resp.Results[0].Status != http.StatusBadRequest { + t.Fatalf("BatchDelete returned results %+v, want one bad request", resp.Results) + } +} + +func TestQueryRejectsOverflowingNeedleDelta(t *testing.T) { + err := (&VolumeServer{}).Query(&volume_server_pb.QueryRequest{ + FromFileIds: []string{overflowingNeedleDeltaFid}, + }, nil) + if err == nil || !strings.Contains(err.Error(), "overflows needle id") { + t.Fatalf("Query returned error %v, want needle id overflow", err) + } +} diff --git a/weed/storage/needle/needle.go b/weed/storage/needle/needle.go index 716ef99a7..f4003c5c6 100644 --- a/weed/storage/needle/needle.go +++ b/weed/storage/needle/needle.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "math" "net/http" "strconv" "strings" @@ -135,6 +136,9 @@ func (n *Needle) ParsePath(fid string) (err error) { } if delta != "" { if d, e := strconv.ParseUint(delta, 10, 64); e == nil { + if d > math.MaxUint64-uint64(n.Id) { + return fmt.Errorf("delta %d overflows needle id %x", d, n.Id) + } n.Id += Uint64ToNeedleId(d) } else { return e diff --git a/weed/storage/needle/needle_test.go b/weed/storage/needle/needle_test.go index 3d6329cca..cc26353eb 100644 --- a/weed/storage/needle/needle_test.go +++ b/weed/storage/needle/needle_test.go @@ -40,6 +40,14 @@ func TestParseKeyHash(t *testing.T) { } } +func TestNeedleParsePathRejectsDeltaOverflow(t *testing.T) { + var n Needle + err := n.ParsePath("ffffffffffffffff00000000_1") + if err == nil { + t.Fatalf("ParsePath accepted overflowing delta with needle id %d", n.Id) + } +} + func BenchmarkParseKeyHash(b *testing.B) { b.ReportAllocs()