Skip to content

Commit 0505e2a

Browse files
authored
fix(fileservice): validate remote cache payloads (#25969)
The remote-cache RPC boundary trusted request shape and response indexes. Schema-valid requests with an empty key list or an omitted nested `CacheKey` could panic the serving CN. A malformed or inconsistent response could use a negative/out-of-range index, an index requested from a different target, or an incorrect data length to panic the requester or mark invalid bytes as a hit. Validate every request entry before allocating the I/O vector, require all keys in one request to name the same file, and return `bad request` without installing a response release callback on invalid input. If `ReadCache` partially acquires cache data and then fails, release it immediately. On the client, accept only indexes sent to that target, reject out-of-bounds and wrong-length hits, and let the normal local-storage path handle every invalid response. An invalid hit does not consume its index, while the first fully valid response wins. The MORPC response is released on all received response paths. Normal same-version senders already generate valid shapes; nil repeated message pointers are only possible for direct in-process callers. The network- relevant cases are empty/missing nested request fields and invalid scalar response indexes/lengths. Validation: - focused fileservice regressions: PASS - focused fileservice regressions with `-race -count=10`: PASS - `.agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=180s ./pkg/fileservice` - `.agents/skills/mo-dev/scripts/mo-cgo-test -race -count=1 -timeout=240s ./pkg/fileservice` - `.agents/skills/mo-dev/scripts/mo-cgo-test -count=1 -timeout=240s ./pkg/cnservice` - `go build ./pkg/fileservice ./pkg/cnservice` (repository CGo environment) - `go vet ./pkg/fileservice ./pkg/cnservice` (repository CGo environment) Approved by: @gouhongshen, @XuPeng-SH
1 parent 647cbe6 commit 0505e2a

3 files changed

Lines changed: 331 additions & 13 deletions

File tree

pkg/cnservice/server_query_test.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1187,7 +1187,6 @@ func Test_service_handleGetCacheData(t *testing.T) {
11871187
require.NoError(t, err)
11881188

11891189
mockQuery := mock_query.NewMockQueryService(ctl)
1190-
mockQuery.EXPECT().SetReleaseFunc(gomock.Any(), gomock.Any()).Return().AnyTimes()
11911190

11921191
type fields struct {
11931192
fileService fileservice.FileService
@@ -1230,7 +1229,7 @@ func Test_service_handleGetCacheData(t *testing.T) {
12301229
want: &query.Response{},
12311230
},
12321231
{
1233-
name: "read_empty",
1232+
name: "missing_cache_key",
12341233
fields: fields{
12351234
fileService: fs,
12361235
queryService: mockQuery,
@@ -1249,7 +1248,7 @@ func Test_service_handleGetCacheData(t *testing.T) {
12491248
},
12501249
resp: &query.Response{},
12511250
},
1252-
wantErr: nil,
1251+
wantErr: dummyBadRequestErr,
12531252
want: &query.Response{GetCacheDataResponse: nil},
12541253
},
12551254
}

pkg/fileservice/remote_cache.go

Lines changed: 36 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -114,26 +114,47 @@ func (r *RemoteCache) Read(ctx context.Context, vector *IOVector) error {
114114
req.GetCacheDataRequest = &query.GetCacheDataRequest{
115115
RequestCacheKey: key,
116116
}
117+
requestedIndexes := make(map[int]struct{}, len(key))
118+
for _, cacheKey := range key {
119+
requestedIndexes[int(cacheKey.Index)] = struct{}{}
120+
}
117121

118122
func(ctx context.Context) {
119123
ctx, cancel := context.WithTimeoutCause(ctx, time.Second*2, moerr.CauseRemoteCacheRead)
120124
defer cancel()
121125
resp, err := r.client.SendMessage(ctx, target, req)
122-
if err != nil {
126+
if err != nil || resp == nil {
123127
// Do not return error here to read data from local storage.
124128
return
125129
}
126130
defer r.client.Release(resp)
127131
if resp.GetCacheDataResponse != nil {
132+
seen := make(map[int]struct{}, len(requestedIndexes))
128133
for _, cacheData := range resp.GetCacheDataResponse.ResponseCacheData {
129-
numRead++
134+
if cacheData == nil || cacheData.Index < 0 {
135+
continue
136+
}
130137
idx := int(cacheData.Index)
138+
if idx >= len(vector.Entries) {
139+
continue
140+
}
141+
if _, ok := requestedIndexes[idx]; !ok {
142+
continue
143+
}
144+
if _, ok := seen[idx]; ok {
145+
continue
146+
}
131147
if cacheData.Hit {
148+
if int64(len(cacheData.Data)) != vector.Entries[idx].Size {
149+
continue
150+
}
132151
vector.Entries[idx].done = true
133152
vector.Entries[idx].CachedData = &Bytes{bytes: cacheData.Data}
134153
vector.Entries[idx].fromCache = r
135154
numHit++
136155
}
156+
seen[idx] = struct{}{}
157+
numRead++
137158
}
138159
}
139160
}(ctx)
@@ -162,27 +183,32 @@ func (r *RemoteCache) Close(ctx context.Context) {
162183
func HandleRemoteRead(
163184
ctx context.Context, fs FileService, req *query.Request, resp *query.WrappedResponse,
164185
) error {
165-
if req.GetCacheDataRequest == nil {
186+
if req == nil || req.GetCacheDataRequest == nil {
166187
return moerr.NewInternalError(ctx, "bad request")
167188
}
168-
first := req.GetCacheDataRequest.RequestCacheKey[0].CacheKey
169-
if first == nil { // We cannot get the first one.
170-
return nil
189+
keys := req.GetCacheDataRequest.RequestCacheKey
190+
if len(keys) == 0 || keys[0] == nil || keys[0].CacheKey == nil {
191+
return moerr.NewInternalError(ctx, "bad request")
171192
}
193+
first := keys[0].CacheKey
172194

173195
ioVec := &IOVector{
174196
FilePath: first.Path,
175197
}
176-
ioVec.Entries = make([]IOEntry, len(req.GetCacheDataRequest.RequestCacheKey))
177-
for i, k := range req.GetCacheDataRequest.RequestCacheKey {
198+
ioVec.Entries = make([]IOEntry, len(keys))
199+
for i, k := range keys {
200+
if k == nil || k.CacheKey == nil || k.CacheKey.Path != first.Path {
201+
return moerr.NewInternalError(ctx, "bad request")
202+
}
178203
ioVec.Entries[i].Offset = k.CacheKey.Offset
179204
ioVec.Entries[i].Size = k.CacheKey.Sz
180205
}
181206
if err := fs.ReadCache(ctx, ioVec); err != nil {
207+
ioVec.Release()
182208
return err
183209
}
184-
respData := make([]*query.ResponseCacheData, len(req.GetCacheDataRequest.RequestCacheKey))
185-
for i, k := range req.GetCacheDataRequest.RequestCacheKey {
210+
respData := make([]*query.ResponseCacheData, len(keys))
211+
for i, k := range keys {
186212
var data []byte
187213
if ioVec.Entries[i].CachedData != nil {
188214
data = ioVec.Entries[i].CachedData.Bytes()

0 commit comments

Comments
 (0)