Skip to content

ensure copy of p when Writing stays within bounds#3

Merged
djherbis merged 1 commit intodjherbis:masterfrom
danp:ensure-bounds
Jan 17, 2017
Merged

ensure copy of p when Writing stays within bounds#3
djherbis merged 1 commit intodjherbis:masterfrom
danp:ensure-bounds

Conversation

@danp
Copy link
Copy Markdown
Contributor

@danp danp commented Jan 17, 2017

I believe calling gap can race which can cause trying to access beyond the end of p. This came about via oklog/oklog#16.

With this patch I can run my test setup indefinitely. I also had debugging inside the if body which showed this at work:

len(p)=30 n=0 gap(w.b)=1048576 nn=30
len(p)=30 n=0 gap(w.b)=1048576 nn=30
len(p)=30 n=0 gap(w.b)=1048576 nn=30
len(p)=30 n=0 gap(w.b)=1048576 nn=30
len(p)=30 n=0 gap(w.b)=1048576 nn=30
len(p)=30 n=0 gap(w.b)=1048576 nn=30
len(p)=30 n=0 gap(w.b)=1048576 nn=30
len(p)=30 n=0 gap(w.b)=1048576 nn=30
len(p)=30 n=0 gap(w.b)=1048576 nn=30
len(p)=30 n=0 gap(w.b)=1048576 nn=30
len(p)=30 n=0 gap(w.b)=1048576 nn=30

@danp danp mentioned this pull request Jan 17, 2017
Closed
@djherbis
Copy link
Copy Markdown
Owner

djherbis commented Jan 17, 2017

Very nice catch, you're right that gap could grow beyond the size of p after a wait!

I think I have a few comments to make, but this is def. a good fix thanks!

@djherbis djherbis self-assigned this Jan 17, 2017
@djherbis djherbis self-requested a review January 17, 2017 15:56
@djherbis djherbis added the bug label Jan 17, 2017
djherbis
djherbis reviewed Jan 17, 2017
View reviewed changes
sync.go
}

// chunk write to fill space
m, err = w.b.Write(p[n : int64(n)+gap(w.b)])
Copy link
Copy Markdown
Owner

@djherbis djherbis Jan 17, 2017

Choose a reason for hiding this comment

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

Why don't we break out this inner for loop if gap(w.b) >= int64(len(p[n:]), it'll then fall through to just do a std. write, and the write should fit entirely in the buffer (we won't lose/regain the lock again).

Copy link
Copy Markdown
Owner

@djherbis djherbis Jan 17, 2017

Choose a reason for hiding this comment

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

Also, do you mind re-basing (I only modified the travis file) so that travis will run correctly?

(not sure why my travis script broke over time)

Copy link
Copy Markdown
Contributor Author

@danp danp Jan 17, 2017

Choose a reason for hiding this comment

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

Updated, does it look like what you were thinking? Seems to still pass my test.

Copy link
Copy Markdown
Owner

@djherbis djherbis Jan 17, 2017

Choose a reason for hiding this comment

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

Looks good, merged it in and included it in release v2.0.1.

I'll probably try and add some more tests to hit this case/boost coverage. It's hard to test toctou conditions reliably. Kicking myself that I missed this, but thankful you caught it!

@djherbis djherbis merged commit ef1ef18 into djherbis:master Jan 17, 2017
@danp danp deleted the ensure-bounds branch January 17, 2017 16:12
@djherbis
Copy link
Copy Markdown
Owner

djherbis commented Jan 17, 2017 •
edited
Loading

Wow that was blazing fast! Thank you very much for the fix :)

I hope you're enjoying the library overall, sorry that it had a bug!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@djherbis djherbis djherbis left review comments

Assignees

@djherbis djherbis

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@danp @djherbis