MSan: Uninitialized value in nsinstall.c

RESOLVED FIXED in mozilla34

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: decoder, Assigned: decoder)

Tracking

(Blocks 1 bug)

Trunk
mozilla34
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

MSan reports a use of uninitialized memory at

http://mxr.mozilla.org/mozilla-central/source/config/nsinstall.c#417

It seems that the parentheses are set wrong, such that the comparison in that line, involving "tosb", is executed, even if !exists. However, !exists means that "tosb" remains uninitialized because the destination file does not exist (see http://mxr.mozilla.org/mozilla-central/source/config/nsinstall.c#140).

If the uninitialized memory is 0, the tool will do what it's supposed to do anyhow though. That might explain why it does not fail.

Reading that if statement, I guess the whole statement should evaluate to false if !exists.

Nathan, could you quickly cross-check that before I make a patch?
Analysis makes sense to me, though that is some gnarly code in that file.
nsinstall.c needs to DIAF. I dare you to run blame on that file :)
Patch. Will do a try run though to make sure this doesn't break anything.
Why are you running Msan on stuff that are is part of Firefox?
MSan is a compile-time instrumentation, everything is compiled with MSan, including our own tools (and there is no reason not to do that, I guess).
(In reply to Christian Holler (:decoder) from comment #5)
> MSan is a compile-time instrumentation, everything is compiled with MSan,
> including our own tools

I don't think the flags enabling it should end up in HOST_C*FLAGS.

> (and there is no reason not to do that, I guess).

There's not much reason to care. Especially about nsinstall.
(In reply to Mike Hommey [:glandium] from comment #6)
> 
> There's not much reason to care. Especially about nsinstall.

I think if the bug described in comment 0 is real, then there is a reason to care: It can fail incremental builds (that's what Jesse suggested). I have no clue if the bug has real impact, but if there is a real bug, then we should fix it, not ignore it.
FWIW,  this can only fail in an incremental build if having nsinstall create a symlink and the destination file exists as a file or directory, not a symlink. Which essentially would involve switching from NSDISTMODE = copy to NSDISTMODE = symlink, which is a very unlikely event.
Comment on attachment 8476105 [details] [diff] [review]
nsinstall-1056233.patch

Green on try with a few builds and tests picked :)
Attachment #8476105 - Flags: review?(nfroyd)
Comment on attachment 8476105 [details] [diff] [review]
nsinstall-1056233.patch

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

rs=me
Attachment #8476105 - Flags: review?(nfroyd) → review+
Blocks: 1058500
https://hg.mozilla.org/mozilla-central/rev/5ca3c0880050
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.