Closed Bug 1428568 Opened 6 years ago Closed 6 years ago

Stop using GetNativePath in libjar

Categories

(Core :: Networking: JAR, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file)

(In reply to Honza Bambas (:mayhemer) from bug 685236 comment #65)
> > if (rv == NS_OK || !outFile)
> >    return rv;
> 
> if (...) { ... } please, when you are here

Fixed locally.

> > #ifdef XP_WIN
> >     DeleteFileW(outFile->NativePath().get());
> > #else
> >     PR_Delete(outFile->NativePath().get());
> > #endif
> > 
> 
> I think deleting |outname| always was a huge and probably even potentially
> security sensitive bug!
> 
> I don't understand why it's even passed in here, since it always has been a
> const c-string held by nsAutoCString.  That must never be deleted directly!
> 
> This weird code has been introduced in bug 214672 [1][2], probably slipped
> among those twenty three versions of that huge patch...
> 
> hence, please remove passing the path down as an arg to ExtractFile
> altogether.

PR_Delete does not deleting the passed pointer. It is a portable version of unlink. Maybe the function name should have been PR_DeleteFile.
I will add a generic wrapper instead of adding #ifdefs everywhere as some reviewers requested in bug 685236.
Priority: -- → P2
Whiteboard: [necko-triaged]
(In reply to Masatoshi Kimura [:emk] from comment #0)
> PR_Delete does not deleting the passed pointer. It is a portable version of
> unlink. Maybe the function name should have been PR_DeleteFile.
> I will add a generic wrapper instead of adding #ifdefs everywhere as some
> reviewers requested in bug 685236.

Ah!  I misread that as some other form of "delete-ptr" function.  Right.  Thanks.
Comment on attachment 8949023 [details]
Bug 1428568 - Stop using GetNativePath in libjar.

https://reviewboard.mozilla.org/r/218418/#review224888

check all places are updated

::: modules/libjar/nsZipArchive.cpp:518
(Diff revision 1)
>    if (aFd) {
>      PR_Close(aFd);
> -    if (rv != NS_OK)
> -      PR_Delete(outname);
> +    if (rv == NS_OK || !outFile) {
> +      return rv;
> +    }
> +    outFile->Remove(false);

rather just:

if (NS_FAILED(rv) && outFile) {
  outFile->Remove(false);
}
Attachment #8949023 - Flags: review?(honzab.moz) → review+
Blocks: 1428258
No longer blocks: 685236
Comment on attachment 8949023 [details]
Bug 1428568 - Stop using GetNativePath in libjar.

https://reviewboard.mozilla.org/r/218418/#review224888

> rather just:
> 
> if (NS_FAILED(rv) && outFile) {
>   outFile->Remove(false);
> }

Fixed.
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/0bb9c745a9df
Stop using GetNativePath in libjar. r=mayhemer
https://hg.mozilla.org/mozilla-central/rev/0bb9c745a9df
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.