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.

Sign up for GitHub

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

Jump to bottom

Update some errors of bugsabsh #113

Closed
wants to merge 1 commit into from
Closed

Update some errors of bugsabsh #113

wants to merge 1 commit into from

Conversation

soniasingla
Copy link
Contributor

Signed-off-by: Sonia Singla soniasingla.1812@gmail.com

@soniasingla soniasingla requested a review from a team as a code owner May 6, 2021 18:24
yaelharel
yaelharel reviewed May 10, 2021
View reviewed changes
local/local.go Outdated Show resolved Hide resolved
yaelharel
yaelharel reviewed May 13, 2021
View reviewed changes
local/local.go Outdated Show resolved Hide resolved
natalieparellano
natalieparellano reviewed May 14, 2021
View reviewed changes
local/local.go Outdated
if _, err := io.CopyN(fh, tr); err != nil {
=======
if _, err := io.CopyN(fh, tr, 1024) {
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Could we move this to the previous line?

We'll also need to resolve the merge conflict :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

@soniasingla soniasingla May 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

thanks for help

@soniasingla
Update some errors of bugsbash
6e4325e 
Signed-off-by: Sonia Singla <soniasingla.1812@gmail.com>
@soniasingla
Copy link
Contributor Author

@natalieparellano can you help me i did not get why test case is failing :/

@natalieparellano natalieparellano mentioned this pull request May 19, 2021
Open
@natalieparellano
Copy link
Member

@soniasingla thanks for updating the PR! I apologize, I didn't have a good grasp before on what io.CopyN is doing. In the current implementation, we are reading only 1024 bytes total - hence the unexpected EOF failures in the tests.

We could put the call to io.CopyN in a for loop as described here and this would satisfy muse-dev (maybe) however it's not any better at preventing DOS attack vs. io.Copy, since we are still blindly copying the whole image. We would need to come up with some reasonable limit on the total number of bytes we expect to read in these scenarios.

I created an issue #120 to capture this question so that we could discuss as a team and come up with a recommendation. If you have any thoughts here, we would love to hear them!

@natalieparellano natalieparellano mentioned this pull request May 19, 2021
Closed
@yaelharel
Copy link
Contributor

I'm going to close this PR following #113 (comment).
We'll discuss this change as part of #120.

@yaelharel yaelharel closed this Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natalieparellano natalieparellano Awaiting requested review from natalieparellano

@yaelharel yaelharel Awaiting requested review from yaelharel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@soniasingla @natalieparellano @yaelharel