1723945 - nsSystemInfo.cpp: On non-x86_{32,64} Linux, CollectProcessInfo can silently fail and insert junk data into the process

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Julian Seward [:jseward]]

-- Summary --

This doesn't happen on any P1 platforms. It affects all non-x86/x86_64 Linuxes
though, in this example, aarch64-linux. It causes Valgrind runs on
aarch64-linux to report hundreds of errors, some of which are shown in the
attached log. These make it more difficult to use Valgrind to look for
problems in aarch64-specific parts of our code base.

-- Details --

What I think happens is:

xpcom/base/nsSystemInfo.cpp
nsSystemInfo::GetProcessInfo(JSContext*, Promise**)

It creates a new PromiseInfo, around line 1531:

    mProcessInfoPromise = InvokeAsync(backgroundET, __func__, []() {
      ProcessInfo info;                       <--------- UNINITIALISED
      nsresult rv = CollectProcessInfo(info);
      if (NS_SUCCEEDED(rv)) {
        return ProcessInfoPromise::CreateAndResolve(info, __func__);
      }
      return ProcessInfoPromise::CreateAndReject(rv, __func__);
    });
  };

A reference to it is passed to CollectProcessInfo(..). That is supposed to
fill in various fields of it, but fails to do so, because ..

(reading the XP_LINUX non-ANDROID implementation of it starting at line 727)

.. it tries to pull out various fields from /proc/cpuinfo, in particular
vendor_id, cpu family, model, stepping, cpu cores etc. That works
fine on x86/x86_64-linux, but on aarch64-linux (or Linux on any other
architecture) the format of /proc/cpuinfo is completely different. See the
second attachment.

Hence attempts to read those fields fail, and the control flow that follows
(at the bottom of CollectProcessInfo(..)) does not alter the fields of the
provided ProcessInfo struct. But nobody detects the failure, and so the
contents of the struct are used uninitialised, resulting in Valgrind reporting
dozens of errors throughout Gecko.

One simple way to fix this is to provide a constructor for struct ProcessInfo
that zeroes out all the fields at the start, hence providing a
safe fallback situation. Frankly I'm amazed that we still allow
classes/structs with no constructors. Don't we have static analysis in
automation to guard against this?

Comment 1

[Julian Seward [:jseward]]

Attachment: Selected valgrind complainage

Comment 2

[Julian Seward [:jseward]]

Attachment: /proc/cpuinfo examples, x86_64 vs aarch64

Comment 3

[Julian Seward [:jseward]]

Attachment: A possible fix

Comment 4

[Julian Seward [:jseward]]

Attachment: Bug 1723945 - nsSystemInfo.cpp: On non-x86_{32,64} Linux, CollectProcessInfo can silently fail and insert junk data into the process. r=nika.

This doesn't happen on any P1 platforms. It affects all non-x86/x86_64
Linuxes though, in this example, aarch64-linux. It causes Valgrind runs on
aarch64-linux to report hundreds of errors. In short, attempts to read
various fields from /proc/cpuinfo fail, because CollectProcessInfo() assumes
that it is looking at a /proc/cpuinfo file from a x86_{32,64} target, but its
format is very different on non x86 targets. Unfortunately the parsing fails,
but the failure is not detected, resulting in the uninitialised fields being
treated as if they contained real data.

The simple fix here is just to add a constructor for class ProcessInfo.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd6051c0a723
nsSystemInfo.cpp: On non-x86_{32,64} Linux, CollectProcessInfo can silently fail and insert junk data into the process.  r=nika.

Comment 6

[Natalia Csoregi [:nataliaCs]]

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

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 8

[Julian Seward [:jseward]]

Beta is also affected. Given that this is non-Tier-1 Linux, I don't think it's worth
uplifting the fix.

Comment 9

[Mike Hommey [:glandium]]

We might actually want it for downstreams, on beta and esr.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is this ready for an ESR uplift request?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)

Is this ready for an ESR uplift request?

Comment 12

[Julian Seward [:jseward]]

On consideration, I would prefer to wontfix esr91, since

• we are not aware of any actual problems caused by the bug

• this is non-P1 Linux only (eg, arm64-linux etc)

• For those targets, I don't have any simple way to know what the effects of changing the reported values of various CPU parameters from <random junk in memory> to zero are, and I don't want to rock the ESR boat.