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

jeffkaufman wins! #217

Closed
jeffkaufman opened this Issue Feb 7, 2019 · 7 comments

Comments

Projects
None yet
4 participants
@jeffkaufman
Copy link
Owner

jeffkaufman commented Feb 7, 2019

screen shot 2019-02-07 at 1 35 53 am

Multiple PRs that all create the same identical file don't conflict. I initially thought I needed a zillion PRs for this, and was churning my way through the automation when I realized I could do it with just #214 and #216.

Going to sleep now.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Feb 7, 2019

I was trying to figure out if this was only a timing issue or not, hence my interference with #215.

I'm also confused about how you managed to merge a PR that still had a queued build at the time of the merge.

This could be argued as a reason for going back to all points in one file, but I actually think it's an argument for removing the non-conflicting PRs can be merged without updating from master exception.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 7, 2019

I was trying to figure out if this was only a timing issue or not, hence my interference with #215.

I thought that was probably you, since you merged #214 for me.

When I switched from the "tons of PRs" strategy to "two PRs" I had a zillion outstanding builds getting in the way of evaluating #214 and #215. I was looking at TravisCI in logged-out mode by accident (very sleepy) and couldn't find a way to cancel them. Then you cancelled #215 which got me to try harder, and I realized I was logged out, logged in, and cancelled the builds that were blocking me. Thanks!

Since you can cancel builds faster than they can complete you could have postponed my PRs for as long as you had the patience. Possibly you (or @csvoss on the west coast) would have been willing to stay up longer than I was, at which point you could have replicated the attack and won instead. I'd be even sleepier now if you'd done that.

I'm also confused about how you managed to merge a PR that still had a queued build at the time of the merge.

I waited for the big button on the PR to turn green, and then pressed it. I don't think there was a queued build?

This could be argued as a reason for going back to all points in one file, but I actually think it's an argument for removing the non-conflicting PRs can be merged without updating from master exception.

Yes, I think we have to remove that exception. While #18 was convenient it turns out it's exploitable.

If this gets really annoying we can implement our own logic for figuring out when out-of-date reviews should still be counted, as proposed in #95

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 7, 2019

I had noticed that two PRs that added the same file weren't seen as conflicting, and realized I could make a bunch of PRs that each took away the same point. By #33 I would still get a point per PR merged, so once they were all in I would be up one point per PR. Since you need 100,000 points to win, that would be 100,000 PRs, which is pretty high! But each one to merge runs the "did someone win" algorithm, so you only need O(sqrt(target)) PRs giving you a point. I figured 1000 should be safe and wrote a little script to create branches testing-1000 through testing-2000 locally. I started another script to push them to GitHub, though at 4s per branch it was going to take about an hour. I started creating PRs for them, which was #112, and then #113 through #213.

Partway through I realized there was a much better way. I could create #214 and #215 which each added 1,000,000 points and removed 1,000,001, but removed the 1,000,001 points using the same file name. git would think there was no merge conflict, and I'd have 1M more points and win. Aside from some interference by @tnelling which meant I had to create #216 instead and get that in, this worked!

Figuring this out ahead of time would have saved me a lot of work, and would also have kept people from noticing what I was doing until after it was too late.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 7, 2019

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Feb 7, 2019

This is super clever!

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Feb 7, 2019

Since you can cancel builds faster than they can complete you could have postponed my PRs for as long as you had the patience. Possibly you (or @csvoss on the west coast) would have been willing to stay up longer than I was, at which point you could have replicated the attack and won instead. I'd be even sleepier now if you'd done that.

I was aware of that at the time, but I wasn't interested in trying to win that way. I only really wanted to find out if GitHub would catch the collision provided the changes weren't both being tested independent of each other - that is, once one PR was merged in, would it still miss the duplicate file creation, or was it a result of getting multiple PRs into a mergeable state simultaneously and then merging them all at once?

It was clear that you could just keep doing what you were doing with another PR, so the answer wouldn't keep you from winning either way, and you're the one who found the exploit, so you earned it.

@csvoss

This comment has been minimized.

Copy link
Contributor

csvoss commented Feb 8, 2019

Hahaha, nice win!

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