1428568 - Stop using GetNativePath in libjar

Status: RESOLVED FIXED

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

Description

[Masatoshi Kimura [:emk]]

(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.

Comment 1

[Honza Bambas (:mayhemer)]

(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 2

[Masatoshi Kimura [:emk]]

Attachment: Bug 1428568 - Stop using GetNativePath in libjar.

mZips key is used only for internal hashtable lookups, so GetPersistentDescriptor is suitable.

Review commit: https://reviewboard.mozilla.org/r/218418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218418/

Comment 3

[Honza Bambas (:mayhemer)]

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);
}

Comment 4

[Masatoshi Kimura [:emk]]

Comment on attachment 8949023 [details]
Bug 1428568 - Stop using GetNativePath in libjar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218418/diff/1-2/

Comment 5

[Masatoshi Kimura [:emk]]

Comment on attachment 8949023 [details]
Bug 1428568 - Stop using GetNativePath in libjar.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218418/diff/2-3/

Comment 6

[Masatoshi Kimura [:emk]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bb9c745a9df
Stop using GetNativePath in libjar. r=mayhemer

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/0bb9c745a9df