6 ms·
I wrote some similar bugs to this in the complex text handling in Chrome. In text layout you do a lot of indexing into various arrays -- like the array of code
by evmar 9y ago
I wrote some similar bugs to this in the complex text handling in Chrome.
In text layout you do a lot of indexing into various arrays -- like the array of code units of the input string, or an array of metadata collected per-code point, or an array of data collected per-grapheme. Oftentimes those arrays are all the same length (like in simple text like Chinese) and mixing up which index to use where is no problem. And then when it goes wrong you're violating array bounds which is a crashable offense.
I agree that it's stupid to have code that causes crashes here. My only defense is that I had a lot of other work to do and complex test pathological cases only affect a fraction of users, none of whom were the ones yelling at me about other bugs. I am rooting for Servo, where they are using a language that defends against bad programmers like me.
PS: If a web page wants to crash, it easily do so by allocating memory in a loop, so making web pages that crash isn't as exciting as it is in Core Text in general. Of course crashes can often be escalated into RCE, but the Chrome sandbox was there to mitigate that.
- gok 9y agoHow would using Rust help this case? An out of bounds array would still lead to a crash, and thus a DoS in the crashing application. You could sandbox the text rendering into its own process to solve that, but then you could do that using unsafe languages anyway.
- tomjakubowski 9y agoRust would significantly help to prevent the escalation of this crash into an RCE.
- GirlsCanCode 9y agoThe Rust zealots are awake!
- mrob 9y agoFully sandboxing an unsafe renderer might have unacceptable performance. E.g. you'd have to reset the internal state after every call, otherwise invalid text on a phishing website might be able to subvert the renderer to make it render text in the URL bar to read something different.
- dom96 9y agoWouldn't any language with exceptions work here? You just define an indexing operator that throws an exception instead of crashing and handle that exception outside the unicode handling function.
- thezoq2 9y agoThe advantage of rusts error handling is that it is explicit. The compiler knows that a function might result in an error and forces the programmer to deal with it, or pass it along. In an exception based language you might forget to deal with the error and have it crash "higher up" in the code. I also suspect that there might be a performance benefit but I could be completely wrong about that
- m0th87 9y ago> The advantage of rusts error handling is that it is explicit. The compiler knows that a function might result in an error and forces the programmer to deal with it, or pass it along. Checked exceptions are explicit as well, though I'm not aware of a language that _only_ has checked exceptions. > I also suspect that there might be a performance benefit but I could be completely wrong about that Yes, this is a big advantage. Stack unwinding is very expensive.
- peoplewindow 9y agoIt's actually slower to not use exceptions and what you describe is not an advantage - exceptions also force you to deal with it or pass it along, if the exception is checked. Bounds check failures aren't of course because that would be incredibly inconvenient and unwieldy, and anyway, you'd just pass it all the way up the stack to some much higher level point which is the only place you can sanely do something (like not render the string at all).
- thezoq2 9y agoAn out of bounds access doesn't have to lead to a crash. For most types in the standard library, the [] operator crashes but the "get" function returns Result<T, E> which you can deal with.
- beojan 9y agoSo it's really no better than C++, where containers have a `.at` function that does bounds-checking and throws an exception if out of bounds.
- cesarb 9y agoThe difference is that on Rust the [] operator does bounds checking: it'll reliably panic before accessing memory if the index is out of bounds. While on C++, the [] operator will happily let the program read or write outside the array bounds. Depending on compile-time options, a Rust panic can either cause an immediate crash, or do something similar to throwing a C++ exception, complete with stack unwinding.
- Manishearth 9y agoYeah, and because it's reliable, you don't need to worry about security issues -- the worst this can do is abort the application. Whereas for this bug it's quite possible that it may be exploitable. Especially given that the crash backtrace doesn't always appear in the same place -- something is corrupting memory that gets discovered later. (This explains why I can sometimes get the string to render for a split second before crashing)
- jopsen 9y ago> PS: If a web page wants to crash, it easily do so by allocating memory in a loop, isn't this also a bug :) And probably want we should fix.. I think all browsers are susceptible, though last I tried in Firefox one had to not just allocate but also fill the memory with garbage.
- cortesoft 9y agoHe means crash the single web page doing the loop... how would you fix this? Isn't the correct behavior when an application goes past resource limits to crash the app? It doesn't crash the browser or any other open tabs.
- wybiral 9y agoBetter process management and limitations between tabs on browsers can help. For instance, this site will crash most Firefox browsers but not Chrome because they limit the modal dialog rates: fan-pages[dot]herokuapp[dot]com (be warned that it may crash your browser even with JS disabled).
- jopsen 9y ago> Isn't the correct behavior when an application goes past resource limits to crash the app? Maybe, or maybe it would be better to exponentially decrease CPU time awarded to a tab over time. And ask the user to confirm that this tab should be allowed to do CPU / memory intensive computations, and otherwise reduce the frame rate, number of ticks, or crash early. Maybe, big memory allocations and CPU intensive stuff should only be allowed on background workers. And even then, we should still reduce resource allocations to save battery, etc. and allow users to award more resources. > He means crash the single web page doing the loop... how would you fix this? It used to be that you could bring my entire system to halt by allocating too much memory in the browser (mostly chrome). Mostly due to swapping and just plain bad system configuration I suspect :)
- cakoose 9y ago> PS: If a web page wants to crash, it easily do so by allocating memory in a loop, so making web pages that crash isn't as exciting as it is in Core Text in general. To allocate memory in a loop, you need some control over the JS. Websites try hard not to serve untrusted JS. But websites serve untrusted text without a second thought. For example, I could post a comment on a news article and cause the article to be unviewable by anyone with a vulnerable browser.
- wybiral 9y agoYou can serve infinite pages like this without JS or weird text issues: fan-pages[dot]herokuapp[dot]com
- LeifCarrotson 9y ago> And then when it goes wrong you're violating array bounds which is a crashable offense. Why is this a crashable offense? Probably a bit naive as a C# application developer, not a system developer, but shouldn't all this code be in the equivalent of a try/catch? If it throws an out-of-bounds error, just log the error and return, rendering what you've got.
- Manishearth 9y agoSystems languages like C++ and C don't usually do error handling that way. You do have try/catch in C++ but folks often disable it. And even if you don't disable it, out of bounds array access will cause segfaults, not exceptions (unless you're using some abstraction over arrays).