1075134 - Implement MOZ_LSAN_INTENTIONALLY_LEAK_OBJECT macro to indicate intentionally leaked objects

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

LeakSanitizer supports Leak Annotations, in http://dxr.mozilla.org/mozilla-central/source/security/sandbox/chromium/base/debug/leak_annotations.h?from=leak_annotations.h#18, but our code can't use it.

I'd like to use it. Pretty please?

Comment 1

[Andrew McCreight [:mccr8]]

What do you mean our code can't use it?

I assume the implementation would be something like MOZ_ASAN_BLACKLIST.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Well, as far as I can tell, and unless I missearched my objdir, leak_annotations.h is not exported anywhere during the build process, so we can't include it. So, unless I'm missing something, we need to reimplement it.

Comment 3

[Andrew McCreight [:mccr8]]

Ah, I misread that and thought __lsan_ignore_object was an annotation for some reason, not a call.

Comment 4

[Andrew McCreight [:mccr8]]

Ok, it looks like it is some function implemented in LSan itself.
  http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130603/176945.html

Does it work if you just declare it somewhere like in leak_annotations?  I don't know how we link with LSan stuff.

Comment 5

[Sergey Matveev]

Those are all declared in public header <sanitizer/lsan_interface.h>. Chromium redeclares them for historical reasons. 

Those symbols should be available whenever you build with -fsanitize=address or -fsanitize=leak.

Comment 6

[Sergey Matveev]

Incidentally, that header also declares __lsan_do_leak_check(), which can be used to move leak detection to an earlier time. I've used that in Chromium to avoid reporting shutdown leaks. I think you've mentioned somewhere that you've been having issues with those, so maybe that approach can work for you too.

Comment 7

[Andrew McCreight [:mccr8]]

Thanks for the explanation, that sounds like it would be pretty easy to implement then.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

I think we're talking past each other. I want to make use of macro ANNOTATE_LEAKING_OBJECT_PTR(X) or something equivalent. We do need to reimplement that macro, don't we?

Comment 9

[Andrew McCreight [:mccr8]]

One thing that makes me a little nervous is that adding easy support for this will make it easier for people to decide themselves that it is okay if they start leaking stuff.  Right now with the suppression file, I get emailed any time somebody makes a change to it.  I guess we can have some kind of comment about requiring a superreview for such changes by the macro.  In practice, nobody has tried to change the suppression file on their own.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #8)
> I think we're talking past each other. I want to make use of macro
> ANNOTATE_LEAKING_OBJECT_PTR(X) or something equivalent. We do need to
> reimplement that macro, don't we?

Yes, I'm discussing the implementation of that macro.  But it sounds simple.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

I'm willing to pick up that bug, if someone is willing to mentor me on it.
The only lsan_interface.h I see is part of clang. Is that the one we are supposed to use?

Comment 11

[Sergey Matveev]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #10)
> I'm willing to pick up that bug, if someone is willing to mentor me on it.
> The only lsan_interface.h I see is part of clang. Is that the one we are
> supposed to use?

Yes.

Comment 12

[Christian Holler (:decoder)]

No, we do not use the Clang header directly. Right now, we add the necessary interface definitions to mfbt/MemoryChecking.h. You can copy any necessary definitions to that file and also define your macros there.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Should I reuse macro MOZ_ASAN or should I introduce a new MOZ_LSAN? If the latter, how do I determine that we are in LeakSanitizer mode?

Comment 14

[Christian Holler (:decoder)]

Typically there is a has_feature macro builtin to Clang for each tool and since LSan is a sub-tool of ASan I would have recommended using that together with MOZ_ASAN to define MOZ_LSAN, like:

http://clang.llvm.org/docs/AddressSanitizer.html#has-feature-address-sanitizer

But on the LeakSanitizer page I don't see a specific macro for that. Maybe you want to try this with a simple program and see if the macro exists. Otherwise, adding MOZ_LSAN directly into the build system is probably ok.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Are you saying I should add --enable-leak-sanitizer? If so, I'll need some guidance, because it's not clear to me how I should go about disabling it cleanly.

Also, the clang headers only contain references to features "address_sanitizer" and "memory_sanitizier", no "leak_sanitizier", so we probably shouldn't rely on it.

Comment 16

[Sergey Matveev]

There is no compile-time check for LeakSanitizer.

You can call those functions whenever you build with ASan. If LSan is not supported (e.g. on Windows or Mac), they will be defined as nops. If you disable LSan at runtime (i.e. detect_leaks=0), then it simply won't do anything with that information.

Comment 17

[Christian Holler (:decoder)]

Given what Sergey is saying, you can just add the LSan macros under MOZ_ASAN then, it will work in both cases then (I forgot that leak checking is a runtime option). If you need more help, you can also ping me on IRC :)

Comment 18

[David Teller [:Yoric] - still alive but not very active]

So, I have an experimental patch, but applying it, I get oranges just about everywhere.
Just to be sure, am I running the right tests?

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b44855a9d005

Comment 19

[Christian Holler (:decoder)]

Needinfo for mccr8 since those seem to be LSan failures.

Comment 20

[Andrew McCreight [:mccr8]]

Yes, that's the right thing to run.

Comment 21

[Andrew McCreight [:mccr8]]

You can also just make an ASan build locally if you have a Linux machine and run any Mochitest and you should get LSan.
  https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Attachment: Introducing MOZ_LSAN_*

Comment 23

[Andrew McCreight [:mccr8]]

Comment on attachment 8503579 [details] [diff] [review]
Introducing MOZ_LSAN_*

Review of attachment 8503579 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable to me, I guess.  It should get a review from an MFBT peer.  It might help if you uploaded the patch in bug 1044020 that shows how you are using these.

As for my concern about adding these to the tree, jst has a script that can watch for the use of certain key words and email people, so we can just add these to the list when it lands.

::: mfbt/MemoryChecking.h
@@ +55,5 @@
>  #define MOZ_MAKE_MEM_DEFINED(addr, size) \
>    __asan_unpoison_memory_region((addr), (size))
> +
> +/*
> + * Thes definitions are usually provided through the

nit: Thes -> These

@@ +56,5 @@
>    __asan_unpoison_memory_region((addr), (size))
> +
> +/*
> + * Thes definitions are usually provided through the
> + * sanitizer/lsan_interface.h header installed by LSan.

This should say "by ASan" not "by LSan", as the latter is really just an execution mode of the former.

@@ +122,5 @@
> +#  define MOZ_LSAN_INTENTIONAL_LEAK(X) /* nothing */
> +#endif // defined(MOZ_ASAN)
> +
> +#if defined(MOZ_ASAN)
> +class ScopedLeakSanitizerDisabler MOZ_FINAL {

What do you need this for?  Does it disable Lsan entirely, or just ignore leaks that are allocated during the disablement?

This needs a comment.

You probably want to indicate that this is a stack-only class.

Comment 24

[Sergey Matveev]

> This should say "by ASan" not "by LSan", as the latter is really just an execution mode of the former.

I think the comment is correct as it is. LSan can be used without ASan, and we plan to also have it in MSan eventually.

Comment 25

[Nathan Froyd [:froydnj]]

Comment on attachment 8503579 [details] [diff] [review]
Introducing MOZ_LSAN_*

Review of attachment 8503579 [details] [diff] [review]:
-----------------------------------------------------------------

Andrew's comments are all good.  I'm suspicious about adding ScopedLeakSanitizerDisable, though; I think that might make it too easy to introduce unintentional leaks.  Do you need it for the dependent bug, or is it just a convenient way of avoiding multiple MOZ_LSAN_INTENTIONAL_LEAKs?  I think forcing people to write out MOZ_LSAN_INTENTIONAL_LEAK is a good thing...

Comment 26

[David Teller [:Yoric] - still alive but not very active]

So, it ignores things that are allocated within the scope.

I need it for the dependent bug:
- `RunWatchdog` calls `PR_Sleep`, which allocates when starting to sleep and deallocates when waking up. Since we generally end the process while the watchdog is sleeping, LSan considers it a leak. The alternative is whitelisting the allocation in `PR_Sleep`, but that strikes me as weirder, since it's a specific call to `PR_Sleep` that we want to whitelist.
- `RunWriter` calls `PR_Open`, writes stuff, then closes. It may also be killed while it's writing. We might whitelist this specific call to `PR_Open`, of course.

Comment 27

[Andrew McCreight [:mccr8]]

David, just so you know, an alternative would be to have some kind of cleanup code that runs at shutdown only in debug and ASan builds.  I'm not sure which is better though.

Comment 28

[David Teller [:Yoric] - still alive but not very active]

I can't think of a way to do this that wouldn't complicate the code of the watchdog too much to my taste. Since this code is important for safety, I'd like to keep it as simple as possible.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Attachment: Introducing MOZ_LSAN_*, v2

Applied feedback.
See also bug 1044020 for updated patch using these macros.

Comment 30

[Andrew McCreight [:mccr8]]

Benjamin, do you think the thing in there about needing a super review is reasonable, or is super reviewing so far in the ash bin of history that we should just rely on the usual reviews plus people's common sense?  (I figure it makes sense to ask a super reviewer about that...)  There's an example of the use in the patch in bug 1044020.

Comment 31

[Benjamin Smedberg]

Comment on attachment 8506976 [details] [diff] [review]
Introducing MOZ_LSAN_*, v2

I think API/design review can still be valuable, yes. In this case, I think we already have the right set of people doing that, so I don't think you should worry about the flag itself. I realize that neither mccr8 nor froydnj are in the official super-reviewer list, but I should probably just figure out a way to fix that.

Comment 32

[Nathan Froyd [:froydnj]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #31)
> I realize that neither mccr8 nor froydnj
> are in the official super-reviewer list, but I should probably just figure
> out a way to fix that.

It's Halloween-time, not April Fools!-time.  OTOH, that's a scary idea, so maybe Halloween is appropriate...

Comment 33

[Nathan Froyd [:froydnj]]

Comment on attachment 8506976 [details] [diff] [review]
Introducing MOZ_LSAN_*, v2

Review of attachment 8506976 [details] [diff] [review]:
-----------------------------------------------------------------

I'm ok with MOZ_LSAN_INTENTIONALLY_LEAK_OBJECT; I think the MOZ_LSAN_INTENTIONAL_SCOPED_LEAK is more of a footgun.

r=me with the MOZ_LSAN_INTENTIONAL_SCOPED_LEAK removed.

::: mfbt/MemoryChecking.h
@@ +113,5 @@
> + * MOZ_LSAN_INTENTIONAL_LEAK(X) is a macro to tell LeakSanitizer that X
> + * points to a value that will intentionally never be deallocated during
> + * the execution of the process.
> + *
> + * Use of this macro requires a super-review.

I think super-review is not the proper category.  Maybe "Additional uses of this macro should be reviewed by people conversant in leak-checking and/or MFBT peers"?

The macro should be named MOZ_LSAN_INTENTIONALLY_LEAK_OBJECT.

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Attachment: Introducing MOZ_LSAN_*, v3

Applied feedback.
Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c579151dd323

Comment 35

[Sergey Matveev]

(In reply to Nathan Froyd (:froydnj) from comment #33)
> Comment on attachment 8506976 [details] [diff] [review]
> Introducing MOZ_LSAN_*, v2
> 
> Review of attachment 8506976 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm ok with MOZ_LSAN_INTENTIONALLY_LEAK_OBJECT; I think the
> MOZ_LSAN_INTENTIONAL_SCOPED_LEAK is more of a footgun.

The scoped annotation is useful mainly for wrapping calls to leaky external code, where you don't have access to the actual leaked object. Of course, that makes "intentional scoped leak" a misnomer.

> 
> r=me with the MOZ_LSAN_INTENTIONAL_SCOPED_LEAK removed.
> 
> ::: mfbt/MemoryChecking.h
> @@ +113,5 @@
> > + * MOZ_LSAN_INTENTIONAL_LEAK(X) is a macro to tell LeakSanitizer that X
> > + * points to a value that will intentionally never be deallocated during
> > + * the execution of the process.
> > + *
> > + * Use of this macro requires a super-review.
> 
> I think super-review is not the proper category.  Maybe "Additional uses of
> this macro should be reviewed by people conversant in leak-checking and/or
> MFBT peers"?
> 
> The macro should be named MOZ_LSAN_INTENTIONALLY_LEAK_OBJECT.

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Perhaps `MOZ_LSAN_INTENTIONALLY_LEAKING_CALL(stmt)` would be less footgunish.
In my sample, this would give

while (true) {
 MOZ_LSAN_INTENTIONALLY_LEAKING_CALL(PR_Sleep(TICK_DURATION));
 // ...
}

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Actually, Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c464d79328cf

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Attachment: Introducing MOZ_LSAN_*, v4

Oops, wrong bug#.

Comment 39

[Andrew McCreight [:mccr8]]

I emailed jst about adding the macro to his watch list.

Comment 40

[Andrew McCreight [:mccr8]]

Thanks for working on this, David, this will be a cool thing to have in our toolkit.

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Thanks for guiding me.

Comment 42

[Carsten Book [:Tomcat]]

Hey Yoric, seems this patch didn't apply cleanly:

applying lsan.diff
patching file mfbt/MemoryChecking.h
Hunk #1 FAILED at 28
1 out of 3 hunks FAILED -- saving rejects to file mfbt/MemoryChecking.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh lsan.diff

could you take a look at this. Thanks!

Comment 43

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rebased

Comment 44

[David Teller [:Yoric] - still alive but not very active]

`hg pull --rebase` seemed to WFM

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e08045200177

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e08045200177

Comment 47

[Andrew McCreight [:mccr8]]

This should get a little bit of documentation on the ASan page, in the LSan section.
  https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer