Skip to content

sn/storage: Fix payload overflow in combined FSTree.Get()#3801

Merged
roman-khimov merged 1 commit intomasterfrom
fstree-combined-get
Feb 6, 2026
Merged

sn/storage: Fix payload overflow in combined FSTree.Get()#3801
roman-khimov merged 1 commit intomasterfrom
fstree-combined-get

Conversation

@cthulhu-rider
Copy link
Contributor

@cthulhu-rider cthulhu-rider commented Feb 4, 2026

i did not try to reproduce this on a cluster. For example, it might show up with

err := c.storage.PutBatch(objs)

@cthulhu-rider cthulhu-rider added bug Something isn't working neofs-storage Storage node application issues labels Feb 4, 2026
@cthulhu-rider cthulhu-rider force-pushed the fstree-combined-get branch 2 times, most recently from 5e1f2c8 to 5908cc2 Compare February 4, 2026 09:57
@codecov
Copy link

codecov bot commented Feb 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 25.55%. Comparing base (b3ee421) to head (0d1c686).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3801      +/-   ##
==========================================
+ Coverage   25.52%   25.55%   +0.02%     
==========================================
  Files         661      661              
  Lines       42641    42647       +6     
==========================================
+ Hits        10884    10897      +13     
+ Misses      30753    30748       -5     
+ Partials     1004     1002       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cthulhu-rider cthulhu-rider marked this pull request as ready for review February 4, 2026 10:04
@roman-khimov roman-khimov added this to the v0.51.1 milestone Feb 4, 2026
Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you find it?

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Not sure why this wasn't noticed previously. We have a lot of tests and production networks.
  2. IIUC we can have excessive data written into combined file already from write cache (not via regular writes). Which changes the game somewhat, technically the data is still here and your fix as it is now would work fine.
  3. Maybe payload-sized LimitReader can be applied layer above.
  4. Not sure if we need a tool to fix it for existing fstrees.

@cthulhu-rider
Copy link
Contributor Author

How did you find it?

Not sure why this wasn't noticed previously. We have a lot of tests and production networks.

noticed by chance while reading this part. Problem was instantly reproduced in unit test, but I didn't try it in devenv. I also didn't see any way the service could mitigate this issue, such as by limiting reading

@roman-khimov
Copy link
Member

IIUC we can have excessive data written into combined file already from write cache

Not true, WC doesn't have combined files, so it produces clean buffers that can be written correctly. This means we don't have broken data on disk, which is good.

@cthulhu-rider
Copy link
Contributor Author

reproduced in devenv

$ neofs-cli object put -r s03.neofs.devenv:8080 -w ../devenv/wallets/wallet.json --cid grnxJmFrMmdEf5ZhzuKaVrdACiDFQbrTeFFxEfYYX5Z --file testfile_16K --no-progress 
Enter password > 
[testfile_16K] Object successfully stored
  OID: 5PMrHej4NCc1J15SPJC6NzRCaHkuwVk6cBEyrwSJVTXr
  CID: grnxJmFrMmdEf5ZhzuKaVrdACiDFQbrTeFFxEfYYX5Z
$
$ neofs-cli object put -r s03.neofs.devenv:8080 -w ../devenv/wallets/wallet.json --cid grnxJmFrMmdEf5ZhzuKaVrdACiDFQbrTeFFxEfYYX5Z --file testfile_16K --no-progress
Enter password > 
[testfile_16K] Object successfully stored
  OID: 4x8nutFJoQ1VFhXbXm5p3H4AA8eTGwQU1a9SMzGv7vZG
  CID: grnxJmFrMmdEf5ZhzuKaVrdACiDFQbrTeFFxEfYYX5Z
$
$ neofs-cli object get -r s03.neofs.devenv:8080 --cid grnxJmFrMmdEf5ZhzuKaVrdACiDFQbrTeFFxEfYYX5Z --oid 4x8nutFJoQ1VFhXbXm5p3H4AA8eTGwQU1a9SMzGv7vZG --ttl 1 --file restored 
 16384 / 16384 [======================================================================================================================================================================================================================================================================================] 100.00% 0s
[restored] Object successfully saved
$
$ neofs-cli object get -r s03.neofs.devenv:8080 --cid grnxJmFrMmdEf5ZhzuKaVrdACiDFQbrTeFFxEfYYX5Z --oid 5PMrHej4NCc1J15SPJC6NzRCaHkuwVk6cBEyrwSJVTXr --ttl 1 --file restored
 32768 / 16384 [======================================================================================================================================================================================================================================================================================] 200.00% 0s
Error: rpc error: copy payload: payload size overflow

To avoid exceeding the payload's boundaries, all callers would have to
perform checks on their side. Although it's more natural to just read
until EOF, and this is what's happening now.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@roman-khimov roman-khimov merged commit bae8501 into master Feb 6, 2026
18 of 22 checks passed
@roman-khimov roman-khimov deleted the fstree-combined-get branch February 6, 2026 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working neofs-storage Storage node application issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants