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: limit max size #15

Merged
merged 2 commits into from
Sep 22, 2022
Merged

fix: limit max size #15

merged 2 commits into from
Sep 22, 2022

Conversation

justindotpub
Copy link
Collaborator

Closes #11

Will need to rebase after #14 is merged.

@justindotpub justindotpub self-assigned this Sep 16, 2022
@lidel lidel changed the title Max file size fix: limit max size Sep 22, 2022
@lidel lidel merged commit 96ba3de into ipfs:main Sep 22, 2022
@justindotpub justindotpub deleted the justincjohnson/max-file-size branch September 22, 2022 13:50
Comment on lines 100 to 111
func Parse(r io.Reader) (rules []Rule, err error) {
s := bufio.NewScanner(r)
// not too large
b, err := bufio.NewReaderSize(r, maxFileSizeInBytes+1).Peek(maxFileSizeInBytes + 1)
if err != nil && err != io.EOF {
return nil, err
}
if len(b) > maxFileSizeInBytes {
return nil, fmt.Errorf("redirects file size cannot exceed %d bytes", maxFileSizeInBytes)
}

s := bufio.NewScanner(bytes.NewReader(b))
for s.Scan() {
Copy link
Member

@lidel lidel Sep 22, 2022

Choose a reason for hiding this comment

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

@Jorropo any room for improvement here? I guess we need to read entire thing anyway, so does not matter much, but asking just to be sure i am not missing something obvious.

Copy link

@Jorropo Jorropo Sep 22, 2022

Choose a reason for hiding this comment

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

I don't see why you use bufio.Reader with a .Peek this large, it's a complex beast inside.

You can just do io.ReadAll(&io.LimitedReader{r, maxFileSizeInBytes+1}).

Copy link

Choose a reason for hiding this comment

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

You can even feed the LimitedReader directly to the scanner if you want.

Copy link

@Jorropo Jorropo Sep 22, 2022

Choose a reason for hiding this comment

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

You could also match rules while reading instead of returning a list of rules, allowing for a fully streaming implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I've switched to simpler code with LimitedReader and streaming in #18

lidel added a commit that referenced this pull request Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define max filesize of accepted _redirects file
3 participants