5 ms·
Well that's embarrassing. Passing a mutable dictionary—containing callbacks that could be closed over heaven-even-knows-what, no less!—between threads? And it's
by bvanderveen 6y ago
Well that's embarrassing. Passing a mutable dictionary—containing callbacks that could be closed over heaven-even-knows-what, no less!—between threads? And it's re-used between requests?
They are lucky this wasn't much, much worse.
Why does the error reporting service need to call the web thread back anyway?
Yet another case where immutable-everything would have prevented this hairball from accreting in the first place.
- matthewaveryusa 6y agoNever underestimate the inertia of working code that makes money.
- arkadiyt 6y ago> Well that's embarrassing. An IDOR is embarrassing - this was a complex bug and Github's investigation/response was great here. I'd be proud if my company handled a security incident like this.
- yarg 6y agoIt can be both.
- brainzap 6y agowhy the acronym
- alpaca128 6y agoFor the people as confused as me, it stands for "Insecure Direct Object Reference" and not "International Day of Radiology" or "Iowa Department of Revenue".
- arghwhat 6y agoCreating such unnecessary complexity that should ahead of time be known to be fragile, and then having these resulting issues, is embarrassing. The follow-up report is good, but it doesn't matter when the conclusion is "our footguns backfired".
- renewiltord 6y agoAs someone who has never added incremental complexity to software until it manifested bugs, I agree.
- strictfp 6y agoI agree, and the bug in the report is likely not the only one caused by these changes.
- andix 6y ago> Well that's embarrassing Probably it wasn't that obvious in the code to see. But if they would've used immutable objects, or at least pure functions with scalar parameters in the first place, such an error would never occur.
- wfleming 6y agoUnicorn isn't GitHub's code, though. So the object-reuse that was the root cause here wasn't their code, and treating your webserver as a working black box is pretty typical (right up until you hit an edge case like this). And Unicorn is pretty old. It wouldn't surprise me if this object reuse dates back to mongrel's code base, back when doing anything with threads in Ruby would have been unusual. It's still bad code on unicorn's part (when I got to that part of the post I said "Unicorn does WHAT?" out loud), and GH should have been more careful introducing threads to an environment that didn't have concurrency before, but every step of the chain is pretty understandable IMHO.