273737 - Fixed possible buffer overrun in nsZipExtractor.cpp

Status: RESOLVED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Tim Ruehsen]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.3; Linux) (KHTML, like Gecko)
Build Identifier: 

- replaced the unconditional use of sprintf() for path generation by snprintf() 
- return error if path buffers are too small 
 

Reproducible: Always
Steps to Reproduce:

Comment 1

[Tim Ruehsen]

Attachment: cvs diff

Comment 2

[Darin Fisher]

use PR_snprintf instead of snprintf.

Comment 3

[Benjamin Smedberg]

Comment on attachment 168216 [details] [diff] [review]
cvs diff

What darin said, use PR_snprintf and make "len" a PRUint32.

>+    if (len<0 || len>=(int)sizeof(apath))

Do you need the cast? and give me a little more whitespace:
if ( len < 0 || len >= sizeof(apath) )

Ditto each time. Other than these little nits, it looks good.

Comment 4

[Tim Ruehsen]

OK, agreed so far. 
But now it starts to become mor ecomplicated: 
To use PR_snprintf() I have to include prprf.h, which in turn includes 
prtypes.h. The typedefs in here conflict with the typedefs in  
dist/include/jar/zipstub.h. gcc output: 
 
In file included from nsZipExtractor.cpp:46: 
../../../../dist/include/jar/zipstub.h:83:1: warning: "PR_PUBLIC_API" redefined 
In file included 
from /usr/oms/src/mozilla/mozilla/dist/include/nspr/prprf.h:57, 
                 from nsZipExtractor.cpp:40: 
/usr/oms/src/mozilla/mozilla/dist/include/nspr/prtypes.h:502:1: warning: this 
is the location of the previous definition 
In file included from nsZipExtractor.cpp:46: 
../../../../dist/include/jar/zipstub.h:92: error: conflicting types for ` 
   typedef long int PRInt32' 
/usr/oms/src/mozilla/mozilla/dist/include/nspr/prtypes.h:325: error: previous 
   declaration as `typedef int PRInt32' 
../../../../dist/include/jar/zipstub.h:93: error: conflicting types for ` 
   typedef long int PROffset32' 
/usr/oms/src/mozilla/mozilla/dist/include/nspr/prtypes.h:420: error: previous 
   declaration as `typedef PRInt32 PROffset32' 
../../../../dist/include/jar/zipstub.h:94: error: conflicting types for ` 
   typedef long unsigned int PRUint32' 
/usr/oms/src/mozilla/mozilla/dist/include/nspr/prtypes.h:324: error: previous 
   declaration as `typedef unsigned int PRUint32' 
../../../../dist/include/jar/zipstub.h:97: error: conflicting types for ` 
   typedef char PRBool' 
/usr/oms/src/mozilla/mozilla/dist/include/nspr/prtypes.h:447: error: previous 
   declaration as `typedef PRIntn PRBool' 
In file included from nsZipExtractor.cpp:46: 
../../../../dist/include/jar/zipstub.h:111:1: warning: "PR_RDONLY" redefined 
In file included 
from /usr/oms/src/mozilla/mozilla/dist/include/nspr/prprf.h:58, 
                 from nsZipExtractor.cpp:40: 
/usr/oms/src/mozilla/mozilla/dist/include/nspr/prio.h:613:1: warning: this is 
the location of the previous definition 
 
Any ideas what to do? 
 

Comment 5

[Tim Ruehsen]

This leads me to the question why we have identical header files in different 
locations (just one example): 
 
oms@blitz-lx:~/src/mozilla/mozilla$ find . -name prprf.h 
./dist/sdk/include/prprf.h 
./dist/include/nspr/prprf.h 
./nsprpub/pr/include/prprf.h 
 
$ 
l ./dist/sdk/include/prprf.h ./dist/include/nspr/prprf.h ./nsprpub/pr/include/prprf.h 
lrwxrwxrwx  1 oms users   37 Dec  8 10:12 ./dist/include/nspr/prprf.h 
-> ../../../nsprpub/pr/include/./prprf.h 
-rw-r--r--  1 oms users 6008 Apr 28  2004 ./dist/sdk/include/prprf.h 
-rw-r--r--  1 oms users 6008 Apr 28  2004 ./nsprpub/pr/include/prprf.h 
 
$ sum ./dist/sdk/include/prprf.h 
33506     6 
$ sum ./nsprpub/pr/include/prprf.h 
33506     6 
 
This seems to be an inconsistency. If it is by purpose, please explain it to 
me. 

Comment 6

[Benjamin Smedberg]

Actually, forget what darin and I said about using the PR_ version; NSPR is not
available to the stub installer. If you can just remake this patch with the
whitespace nits, that's fine.

And to understand why we have multiple copies of header files floating around,
you need to understand that all the source header files are installed to
<objdir>/dist/include/<module>, so that they can be found by using REQUIRES in
the Makefiles. dist/sdk is how we build the gecko SDK, which contains only
frozen header files.

Comment 7

[Tim Ruehsen]

Attachment: cvs diff

changed spacing

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 168296 [details] [diff] [review]
cvs diff

>+            if (-1 == mkdir(currdir, 0755)) return E_MKDIR_FAIL;

Please split that into two lines. sr=dveditz

Comment 9

[Tim Ruehsen]

Attachment: cvs diff for nsZipExtractor.cpp

Comment 10

[Tim Ruehsen]

I'm not shure: should I edit the flags of the new patch? how? 
 

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 170806 [details] [diff] [review]
cvs diff for nsZipExtractor.cpp

Carrying over flags. Thanks.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.8b.

Tim, you may want to ask people (either reviewers or others on irc.mozilla.org
channel #developers) to check in patches when they are set to go...