On Mon, Jul 17, 2017 at 10:01 AM, Kris Maglione <kmagli...@mozilla.com> wrote:
> On Mon, Jul 17, 2017 at 09:52:55AM -0700, Bobby Holley wrote: > >> On Mon, Jul 17, 2017 at 9:42 AM, Benjamin Smedberg <benja...@smedbergs.us >> > >> wrote: >> >> I don't know really anything about how rust panics get reflected into >>> crash-data. Who would be the right person to talk to about that? >>> >>> >> Rust panics are equivalent to MOZ_CRASHES, and we treat them as such (or >> at >> least try to, see bug 1379857). >> >> Rust makes it easier to put non-constant things in the crash strings, >> which >> can be quite useful (see [1] as an example). They're not used often, and >> it >> seems unlikely that the existing use-cases would pose privacy issues, but >> I >> don't have a good proposal for enforcing that. >> > > It would be nice if we could add a commit hook that required a data-r tag > for any changes that add MOZ_CRASH_UNSAFE_PRINTF or a Rust panic with a > non-static string. I suspect it's something that most people will tend to > overlook. For the cases that are clearly not data privacy issues, we could > accept data-r=trivial, or something like that. > Phabricator allows you to create rules when certain events occur. You can react to changes to certain files or when certain content within files changes. Phabricator has the concept of a "blocking reviewer." If set, you must have a review from a "blocking reviewer" before the patch can be accepted (r+'d). A "blocking reviewer" can be an individual or a group. If a group, a member of that group must accept the patch. Putting the two together, Phabricator can be configured to automatically flag diffs containing changes to MOZ_CRASH_UNSAFE_PRINTF so they set a "blocking reviewer" from a "data review" group, thus ensuring data review is conducted before the patch lands. (This also means we could formalize module review in Phabricator if we wanted. And I predict there will be some interesting debates about how formal we want the module review policy to be in a post-Phabricator world and how "blocking reviewer" should be used. But that's for a different thread.) Phabricator's features are superior to custom server-side hooks for various reasons and I look forward to the day when we can define "commit policy" as part of Phabricator rules instead of custom hooks. Of course, we need all commits landing via Phabricator/Autoland before we can talk about abandoning server-side hooks completely. That's in the works for post-Phabricator. I'm unsure of timetable. _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
