nsLocalFileWin is missing a number of result checks

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: mayhemer, Assigned: jaas)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

and I think the same applies to IsDirectory() few lines below.
Assignee: nobody → jaas
Posted patch Fix v1.0Splinter Review
There are a lot of unchecked results in this file and in most (all?) case the value can in fact be uninitialized. This patch fixes a bunch of them with a consistent pattern.

I haven't pushed to try yet because I'm waiting for commit access to get re-enabled.
Attachment #8981752 - Flags: review?(nfroyd)
Comment on attachment 8981752 [details] [diff] [review]
Fix v1.0

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

It looks like we're inconsistent about initializing outparams; Exists(), for instance, always initializes to `false` before checking (we'll ignore the failure case for not passing an outparam).  Whereas IsDirectory(), via HasFileAttributes(), can return without initializing the outparam.  And so on.

In any event, more error checking is more better.  Thanks for the patch!
Attachment #8981752 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #3)

> It looks like we're inconsistent about initializing outparams; Exists(), for
> instance, always initializes to `false` before checking

Exists() can also return without initializing to false. CHECK_mWorkingPath() is a macro that can return from the function and it's the first thing done in Exists().

#define CHECK_mWorkingPath()                    \
    do {                                        \
        if (mWorkingPath.IsEmpty())             \
            return NS_ERROR_NOT_INITIALIZED;    \
    } while(0)
Keywords: checkin-needed
I forgot to add a commit message. Will do that and then mark checkin-needed.
Keywords: checkin-needed
Summary: nsLocalFile::RenameTo doesn't check result of Exists() → nsLocalFileWin is missing a number of result checks
Keywords: checkin-needed
Thanks, Josh!
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27650b5daf68
Add a number of result checks to nsLocalFileWin, avoid use of uninitialized values. r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/27650b5daf68
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.