Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(fr32): correctly calculate the todo size under low core counts #12884

Open
wants to merge 2 commits into
base: fix/unpadreader-panics
Choose a base branch
from

Conversation

tediou5
Copy link

@tediou5 tediou5 commented Feb 10, 2025

Related Issues

Fixes #9324
Based on #12491.

Proposed Changes

Currently, the unpadReader's work size is calculated using MTTresh * mtChunkCount(sz). When the core count is low, this may cause the todo length to exceed len(work).

In the current implementation, the length of out is actually expanded automatically by Go's underlying mechanisms, making adjustments quite troublesome.

// io.go
func ReadAll(r Reader) ([]byte, error) {
  b := make([]byte, 0, 512)
  for {
    n, err := r.Read(b[len(b):cap(b)])
    b = b[:len(b)+n]
    if err != nil { /* ... */ }

    if len(b) == cap(b) {
      // Add more capacity (let append pick how much).
      b = append(b, 0)[:len(b)] // <-- here
    }
  }
}

There are two feasible solutions:

  1. Increase the minimum mtChunkCount to 16.
  2. Ensure that todo does not exceed the size of work.

I'm not entirely sure if other conditions could also lead to similar issues, so for now, I've chosen the second approach. I added a check when calculating todo to prevent overflow:

todo := min(abi.PaddedPieceSize(outTwoPow), abi.PaddedPieceSize(len(r.work)))

Additionally I’ve also fixed the CI.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

@rjan90 rjan90 added the skip/changelog This change does not require CHANGELOG.md update label Feb 10, 2025
@rjan90 rjan90 requested a review from magik6k February 10, 2025 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does not require CHANGELOG.md update
Projects
Status: 🔎 Awaiting Review
Development

Successfully merging this pull request may close these issues.

2 participants