1190479 - Allow testing leaky shutdown in "leak-checking" builds (NS_FREE_PERMANENT_DATA)

Status: NEW

(Core :: XPCOM, enhancement)

Description

[Jesse Ruderman]

In "leak-checking" builds (debug/ASan/Valgrind), a macro called NS_FREE_PERMANENT_DATA is defined. This makes eight C++ files do different things, usually in the form of extra work during shutdown.

I'm worried this means we never do assertion checks or memory-safety checks of the shutdown path used by release builds. (How much should this worry me? How different is shutdown when these things are allowed to "leak"?)

Comment 1

[Jesse Ruderman]

I can think of a few ways to make these checks run-time in debug builds and compile-time in release builds. I'm assuming we create a function called free_permanent_data() that queries an environment variable.

A)
    #ifdef ENABLE_LEAK_CHECKING
        if (free_permanent_data()) { ... }
    #endif
    
B)
    [macro: "false" in release; "free_permanent_data()" in debug]
    if (FREE_PERMANENT_DATA) { ... }

C)
    [macro: "" in release; "if (free_permanent_data()) ..." in debug]
    IF_FREE_PERMANENT_DATA({ ... })

Comment 2

[Benjamin Smedberg]

I have no opinion. Perhaps obviously at some point we want to skip shutdown in release builds and just terminate the process, but we haven't had time to dedicate to that project.

Comment 3

[Jens Stutte [:jstutte]]

Not sure if there is anything we want to do here, but moving the component for now.

Comment 4

[Andrew McCreight [:mccr8]]

I think this is still a legitimate issue, but that it is less of a concern nowadays. In content processes, where all of the scary web pages live, we just kill the process quite early in shutdown, in non-leakchecking builds. There's been some work to bail out earlier in the main process, but I think that stalled out so we still do a good chunk of XPCOM shutdown.

One interesting angle here is that our ASan builds are NS_FREE_PERMANENT_DATA. We need this for LSan, but this means we're hitting this issue for ASan memory safety stuff. One problem related to this that I have noticed is that, sort of the opposite of the initial concern in comment 0, is that various security researchers occasionally report UAFs or whatever in shutdown code that never runs in opt builds. I filed bug 1850021 about changing that for fuzzing ASan builds but I haven't gotten around to it. It doesn't seem to crop up more than a few times a year.