Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss
Re: [dev-servo] PSA: Avoid invoking Debug formatters in release-mode Rust
431 views
Skip to first unread message
Lars Bergstrom
Jan 13, 2018, 9:22:21 AM1/13/18
to dev-servo, dev-platform
At least for Servo, should we add a check to tidy (
https://github.com/servo/servo/blob/master/python/tidy/servo_tidy/tidy.py)
immediately to catch the use of that fairly-unique formatting string, as we
do for a bunch of other random stuff? We can always exempt particular
files/folder where we think it shouldn't matter, but if it's that easy to
footgun we might as well put some simple guards in place.
- Lars
PS - welcome back!
On Fri, Jan 12, 2018 at 11:07 PM, Bobby Holley <bobby...@gmail.com>
wrote:
> TL;DR: To prevent code bloat, avoid {:?} in format strings for panic!(),
> unreachable!(), error!(), warn!(), and info!() for Rust code that ships in
> Gecko.
>
> Longer version:
>
> One nice thing about Rust is that you can #[derive(Debug)] for a type, and
> the compiler will generate a stringification method, which may in turn be
> inductively defined in terms of the Debug implementations of member types.
> This is great for logging and debugging, but the implementations can be
> quite large depending on the types involved.
>
> The linker will generally eliminate unused Debug implementations as dead
> code, but can't do so if they might be invoked in release builds. The most
> common way this seems to happen is in panic!() messages, where it can be
> tempting to include a stringified value to make the message more
> informative. It can also happen for the logging macros that don't get
> compiled out of release builds, which (at least for stylo) are info!(),
> warn!(), and error!() [1].
>
> To demonstrate what's at stake here, this trivial patch eliminates more
> than 80K from libxul: https://github.com/servo/servo/pull/19756
>
> Given how easy it is to mess this up and pull tons of unnecessary code into
> Firefox, and given that it's rather time-consuming to notice the problem
> and track down the culprit, I think we're best off categorically avoiding
> this pattern.
>
> Comments and alternative proposals welcome.
>
> bholley
>
>
> [1]
> https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df
> 076b967ac6/servo/ports/geckolib/Cargo.toml#21
> _______________________________________________
> dev-servo mailing list
> dev-...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-servo
>
https://github.com/servo/servo/blob/master/python/tidy/servo_tidy/tidy.py)
immediately to catch the use of that fairly-unique formatting string, as we
do for a bunch of other random stuff? We can always exempt particular
files/folder where we think it shouldn't matter, but if it's that easy to
footgun we might as well put some simple guards in place.
- Lars
PS - welcome back!
On Fri, Jan 12, 2018 at 11:07 PM, Bobby Holley <bobby...@gmail.com>
wrote:
> TL;DR: To prevent code bloat, avoid {:?} in format strings for panic!(),
> unreachable!(), error!(), warn!(), and info!() for Rust code that ships in
> Gecko.
>
> Longer version:
>
> One nice thing about Rust is that you can #[derive(Debug)] for a type, and
> the compiler will generate a stringification method, which may in turn be
> inductively defined in terms of the Debug implementations of member types.
> This is great for logging and debugging, but the implementations can be
> quite large depending on the types involved.
>
> The linker will generally eliminate unused Debug implementations as dead
> code, but can't do so if they might be invoked in release builds. The most
> common way this seems to happen is in panic!() messages, where it can be
> tempting to include a stringified value to make the message more
> informative. It can also happen for the logging macros that don't get
> compiled out of release builds, which (at least for stylo) are info!(),
> warn!(), and error!() [1].
>
> To demonstrate what's at stake here, this trivial patch eliminates more
> than 80K from libxul: https://github.com/servo/servo/pull/19756
>
> Given how easy it is to mess this up and pull tons of unnecessary code into
> Firefox, and given that it's rather time-consuming to notice the problem
> and track down the culprit, I think we're best off categorically avoiding
> this pattern.
>
> Comments and alternative proposals welcome.
>
> bholley
>
>
> [1]
> https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df
> 076b967ac6/servo/ports/geckolib/Cargo.toml#21
> _______________________________________________
> dev-servo mailing list
> dev-...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-servo
>
Bobby Holley
Jan 15, 2018, 9:14:33 PM1/15/18
to dev-servo, dev-platform
(From your next message it sounds like there's no disagreement here, but I
wanted to get the reasoning written down)
On Sat, Jan 13, 2018 at 6:36 AM, Anthony Ramine <n.o...@gmail.com> wrote:
> I would much rather prefer if we just checked that we didn't use the Debug
> impls of large types.
The issue here is that it's difficult to tell at a glance how big a Debug
impl will be, especially at the call site. Generics make this extra
complicated, and even a couple of "small" stringifiers can easily add up.
Moreover, a case-by-case analysis makes it hard to do enforcement with
tooling. That said, if the tooling supports a whitelist, I'm totally fine
with allowing exceptions when we have a good reason. I'm proposing some
tooling in bug 1430678.
> We could also just not derive them in release mode.
>
I believe debug!() statements are type-checked in release-mode compiles, so
this would cause our logging code to break.
>
> In the PR you link, AFAICT you also removed some uses that were just very
> small enums or even integers, which may not be necessary.
wanted to get the reasoning written down)
On Sat, Jan 13, 2018 at 6:36 AM, Anthony Ramine <n.o...@gmail.com> wrote:
> I would much rather prefer if we just checked that we didn't use the Debug
> impls of large types.
The issue here is that it's difficult to tell at a glance how big a Debug
impl will be, especially at the call site. Generics make this extra
complicated, and even a couple of "small" stringifiers can easily add up.
Moreover, a case-by-case analysis makes it hard to do enforcement with
tooling. That said, if the tooling supports a whitelist, I'm totally fine
with allowing exceptions when we have a good reason. I'm proposing some
tooling in bug 1430678.
> We could also just not derive them in release mode.
>
I believe debug!() statements are type-checked in release-mode compiles, so
this would cause our logging code to break.
>
> In the PR you link, AFAICT you also removed some uses that were just very
> small enums or even integers, which may not be necessary.
>
> > Le 13 janv. 2018 à 06:07, Bobby Holley <bobby...@gmail.com> a écrit :
> >
> > Comments and alternative proposals welcome.
>
> > Le 13 janv. 2018 à 06:07, Bobby Holley <bobby...@gmail.com> a écrit :
> >
> > Comments and alternative proposals welcome.
>
Chris Peterson
Jan 16, 2018, 3:09:10 AM1/16/18
to
On 2018-01-12 9:07 PM, Bobby Holley wrote:
> The most
> common way this seems to happen is in panic!() messages, where it can be
> tempting to include a stringified value to make the message more
> informative.
Just a friendly reminder: panic messages that are parameterized to
> The most
> common way this seems to happen is in panic!() messages, where it can be
> tempting to include a stringified value to make the message more
> informative.
include debug data might expose PII in Firefox crash reports. Patches
that add new parameterized panic messages should probably be reviewed by
a data steward:
https://wiki.mozilla.org/Firefox/Data_Collection
Anthony Ramine
Jan 18, 2018, 2:35:04 PM1/18/18
to dev-servo, dev-platform
That being said, looking at nsCSSParser.cpp in Gecko, I just realised you never had similar code (printing values when some invariant is broken). Given how old Gecko is, I guess that means this is not even useful to begin with (I mean the printing you removed was not useful to begin with, just to be extra clear).
I wouldn't mind a tidy thing choking on the slightest appearance of "{:?}" in those files.
I wouldn't mind a tidy thing choking on the slightest appearance of "{:?}" in those files.
0 new messages