On 2019-10-11 12:13, Ryosuke Niwa wrote: > On Fri, Oct 11, 2019 at 2:09 AM Chris Lord <cl...@igalia.com> wrote: > >> On 2019-10-10 23:15, Ryosuke Niwa wrote: >>> On Thu, Oct 10, 2019 at 2:07 PM Myles C. Maxfield >>> <mmaxfi...@apple.com> wrote: >>> >>>>> On Oct 10, 2019, at 12:57 PM, Ryosuke Niwa <rn...@webkit.org> >>>>> wrote: >>>>> >>>>> Hi Chris, >>>>> >>>>> I'm excited that you're working on OffscreenCanvas because I >> think >>>>> it would be a valuable feature >>>> >>>> Me too!!! >>>> >>>>> , and I'm confident we can come up with a strategy to limit its >>>>> privacy & security risk as we see fit. >>>>> >>>>> However, many of your patches seem to ignore the fact most of >>>>> WebCore objects aren't thread safe. For example, CSS parser and >>>>> the entire CSS object model aren't designed to used in non-main >>>>> thread. Regardless of how ready Linux ports might be, we can't >>>>> land or enable this feature in WebKit until all thread safety >>>>> issues are sorted out. >>>>> >>>>> Unfortunately, I can't make a time commitment to review & find >>>>> every thread safety issue in your patches. Please work with >>>>> relevant experts and go over your code changes. >>>> >>>> I’d be happy to work with you on this. >>>> >>>>> For example, it's never safe to an object that's RefCounted in >>>>> multiple threads because RefCounted isn't thread safe. One would >>>>> have to use ThreadSafeRefCounted. It's never safe to use >>>>> AtomString from one another in another because AtomString has a >>>>> pool of strings per thread. For that matter, it's never safe to >>>>> use a single String object from two or more threads because >> String >>>>> itself is RefCounted and isn't thread safe. It's not not okay to >>>>> do readonly access to basic container types like Vector, >> HashMap, >>>>> etc... because they don't guarantee atomic update of internal >> data >>>>> structures and doing so can result in UAF. >> >> To the best of my knowledge, I've taken this into account (it's hard >> to >> see this without applying the whole patch series, as later patches >> assume the changes made with respect to thread-safety in the earlier >> patches). There are of course bound to be things I've missed - and >> I'm >> also open to taking a different strategy on certain things too, I'm >> fairly new to the WebKit codebase (well, I'm assuming having worked >> on >> it around 10 years ago doesn't apply too much anymore :)) and I >> imagine >> I've taken some naive approaches in places that someone more >> experienced >> would be able to correct me on. > > I think you want Antti's input most here since many of the classes > you're touching in CSS is maintained by Antti these days. > > The thing that worries me most about this feature is the thread > safety. Historically, we had many thread safety issues regarding > Timer, RefCounted objects, WeakPtr, etc... Chris (Dumez) and I have > been addressing some of those issues more systematically (e.g. > https://trac.webkit.org/r241499 & https://trac.webkit.org/r248113) > because we seem to keep making same mistakes across the codebase. The > key here is to really make which objects are used concurrently or in > non-main threads obvious to whomever reading the code in the future. > > I can think of a few ways to do that. For one, we should be adding > lots of thread safety assertions. Assertions are better than comments > because they're obvious to readers and they warn us whenever they get > out of date or we make mistakes. In fact, I discourage adding comments > about thread safety; if you find yourself doing that, then it's time > to refactor code such that the code is obviously multi-threaded or > concurrent by the virtue of the way things are structured or make it > not matter because the code is naturally thread safe. Just as an > example, one of your patches made CSSValuePool's member functions > thread safe but it left the rest of code still access > CSSValuePool::singleton() object. This is problematic because > singleton objects are usually shared across threads. A better approach > is to have each caller explicitly get a specific instance of > CSSValuePool from some other object; e.g. CSSParser's member variable. > This will make it so that the code that needs to run concurrently > would not rely on singleton objects, and whatever new code which gets > introduced to CSSValuePool or CSSParser would naturally be thread safe > so long as the author follows the same convention. Finally, we can add > an assertion in CSSValuePool::singleton() to make sure it's only > called in the thread. This way, anyone who makes a mistake of calling > this singleton function in CSSParser, or elsewhere which can be used > by the offscreen canvas code in worker would hit this assertion.
Just an update on this, CSS parsing largely happens via static functions, so there's no one place to add a CSSValuePool member variable - however, I did add it where applicable and added alternative functions to all of the CSS parsing static functions that end up in using a CSSValuePool with versions of the function that accept a given CSSValuePool. I wonder if we want to remove the versions that don't to prevent accidental usage of CSSValuePool::singleton()? >>>>> I think the hardest part of this project is validating that >>>>> enabling this feature wouldn't introduce dozens of new thread >>>>> safety issues and thereby security vulnerabilities. >>>> >>>> Sounds like this this is a good candidate for a feature flag. >>> >>> Yeah, in fact, this should really have a compile time flag in >> addition >>> to runtime flag, and it should be default off until we've done >> enough >>> fuzzing. The thread unsafe code can be turned into an attack >> gadget if >>> it exists at all in production binary. >> >> This sounds good to me, I'll file a bug and write a patch for this. >> I >> assume there are ways of enabling features when tests are run? While >> a >> user (or a developer) using WebKit wouldn't want this feature to be >> enabled, I think it should be enabled for (some) test runs as soon >> as >> possible to give us an indication of any issues that exist at the >> moment. > > If you added a compile time flag, then you'd have to manually enable > that for testing. Historically, we've done things like adding a > special buildbot builder which runs tests with a certain feature > enabled. You could also opt to turn on the compile time flag on by > default on GTK+ port assuming the rest of GTK+ port maintainers are > okay with that. So I was just looking at adding a setting, but looks like it's already behind a runtime-setting that's controlled by the experimental-features flag, so I guess we're ok as it is? > For this feature, you may also want to setup a bot or some machine > which runs ThreadSanitizer: > https://clang.llvm.org/docs/ThreadSanitizer.html > > I'd definitely recommend doing extensive fuzzing to make sure we've > sorted out all the thread safety issues before turning it on by > default though especially because offscreen canvas isn't a kind of > feature normal layout tests or WPT tests would exercise so just > running them would probably be not enough to catch all nasty thread > safety issues. We're looking into setting up an extra test machine with ThreadSanitizer, we'll have a look at fuzzing too. I'll post when we've made some headway. Cheers, Chris _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
