[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
5 bug/misc. fixes for ppf_PakFile.cc
There is what I would regard as a cosmetic bug in ppf_PakFile::Mount(). At a
point, it checks to see wheter write access is specified in its argument,
and if it is, it checks to see that the pak file in question is of format 1.
If this is not the case, this line gets executed:
ppDebug1 ("PakFile format doesn't support write access");
This isn't strictly correct. The PakFile format does support write access
(or it will), however, it is correct that pakfiles of format 0 does not, so
the line should rather be something like this:
ppDebug1 ("PakFiles of format 0 doesn't support write access");
-------------------------------
There is another thing in ppf_PakFile::Mount() that doesn't really have to
be a bug, but it puzzels me somewhat.
The RDAttribs member of ppf_PakFile has its ppfFA_executable bit set,
nomatter the cursumstances (outside of a failure). Why?
btw, what does the RD in RDAttribs stand for?
-------------------------------
I noticed this little piece of code in ppf_PakFile::Mount():
try {
root_dir = new ppf_PakFileDir (tmpfilename,
this,
93,
0,
RDAttribs,
creation_time,
creation_time);
} catch (ppException &XCept) {
delete[] tmpfilename;
root_dir = 0; // memory leak ??
ppWarning ("ppf_PakFileDir construction failed");
return false;
}
I checked what happends if an exception is thrown in a constructor by
creating a little test program (attached). Setting root_dir to 0 is not only
not a memory leak, it is also redundant, as the value of root_dir is never
altered in this piece of code if an exception is thrown from
ppf_PakFileDir::ppf_PakFileDir().
There is also another thing with this code, though: If you are not going to
use a variable, would you please not name it? (here I'm talking about XCept)
If you do name it, but don't use it, MS VC++ makes warnings that there is a
variable that is not used. This is perfectly ok behavior normally, but in
this case, its a little annoying, and a client trying to compile PenguinFile
would also get it if his compiler incorporated this feature.
-------------------------------
The first line of ppf_PakFile::Mount() is:
char *tmpfilename = ppf_RemoveExtension (filename);
The caller of ppf_RemoveExtension() must itself delete[] the returned
string.
tmpfilename is under normal circumstances not deleted in
ppf_PakFile::Mount(). It is, however, used as the first argument in a
constructor call to create a new ppf_PakFileDir on the heap. If this
constructor throws an exception, then tmpfilename is deleted (look at the
code from the previous section).
The called constructor of ppf_PakFileDir does not do anything with its first
argument, but merely passes it on to its base class' constructor
(ppf_Directory) as the first parameter.
Now, what happends in the called constructor of ppf_Directory is that a copy
is made of the first parameter. It is *not*, however, deleted: We have a
memory leak. tmpfile should be delete[]d in ppf_PakFile::Mount(), regardless
of wheter or not the called constructor of ppf_PakFileDir throws an
exception.
Also, tmpfilename should not be initialised before its needed, as there is
several places that can cause ppf_PakFile::Mount() to return before the
point it is used. It currently is initialised at the very top of
ppf_PakFile::Mount().
-------------------------------
The member variable handle (the name of the variable is handle) of
ppf_PakFile is in the constructor ppf_PakFile::ppf_PakFile() initialised to
0. This constructor then proceeds to call ReadHeader() without doing
anything more to the member variable handle.
The problem is that ReadHeader() expects the member variable handle to be a
handle to an open file which header it can read...
ppf_PakFile::ppf_PakFile() should open a file and put the handle in the
member variable handle before calling ReadHeader().
begin 666 ctorexcep.cc
M(VEN8VQU9&4@/&EO<W1R96%M+F@^#0H-"F-L87-S(&5X8V5P#0I[#0I].PT*
M#0IC;&%S<R!##0I[#0IP=6)L:6,Z#0H)0R@I#0H)>PT*"0EC;W5T(#P\("))
M;B!#.CI#*"DN(B \/"!E;F1L.PT*#0H)"71H<F]W(&5X8V5P*"D[#0H)"6-O
M=70@/#P@(E1H<F]W:6YG(&5X8V5P=&EO;B!F<F]M($,Z.D,H*2XB(#P\(&5N
M9&P[#0H-"@D)8V]U=" \/" B0SHZ0R@I(')E='5R;FEN9R(@/#P@96YD;#L-
M"@E]#0H-"@E^0R@I#0H)>PT*"0EC;W5T(#P\("));B!#.CI^0RXB(#P\(&5N
M9&P[#0H)?0T*#0H)=F]I9"H@;W!E<F%T;W(@;F5W*'-I>F5?="!S:7IE*0T*
M"7L-"@D)8V]U=" \/" B0V%L;&EN9R!O<&5R871O<B!N97<H*2XB(#P\(&5N
M9&P[#0H)"6-O=70@/#P@(E9A;'5E(&]F('!A<F%M.B B(#P\('-I>F4@/#P@
M96YD;#L-"@D)=F]I9"H@<%1M<" ](#HZ;W!E<F%T;W(@;F5W*'-I>F4I.PT*
M"0EC;W5T(#P\(")2971U<FX@=F%L=64@9G)O;2!O<&5R871O<B!N97<H*3H@
M(B \/"!P5&UP(#P\(&5N9&P[#0H)"6-O=70@/#P@(F]P97)A=&]R(&YE=R@I
M(')E='5R;F5D+B(@/#P@96YD;#L-"@D)<F5T=7)N('!4;7 [#0H)?0T*#0H)
M=F]I9"!O<&5R871O<B!D96QE=&4H=F]I9"H@<$,I#0H)>PT*"0EC;W5T(#P\
M(")C86QL:6YG(&]P97)A=&]R(&1E;&5T92@I+B(@/#P@96YD;#L-"@D)8V]U
M=" \/" B5F%L=64@;V8@<&%R86TZ("(@/#P@<$,@/#P@96YD;#L-"@D).CIO
M<&5R871O<B!D96QE=&4H<$,I.PT*"0EC;W5T(#P\(")O<&5R871O<B!D96QE
M=&4H*2!R971U<FYE9"XB(#P\(&5N9&P[#0H)?0T*?3L-"@T*=F]I9"!M86EN
M*"D-"GL-"@EC;W5T(#P\(")3=&%R=&EN9R!P<F]G<F%M+B(@/#P@96YD;#L-
M"@T*"4,J('!#(#T@,#L-"@EC;W5T(#P\(")686QU92!O9B!P0SH@(B \/"!P
M0R \/"!E;F1L.PT*#0H)=')Y#0H)>PT*"0EC;W5T(#P\(")297%U97-T:6YG
M(&YE=R!#(&AE87 @;V)J96-T+B(@/#P@96YD;#L-"@D)<$,@/2!N97<@0SL-
M"@D)8V]U=" \/" B4F5Q=65S="!S871I<V9I960N(B \/"!E;F1L.PT*"7T-
M"@EC871C:" H97AC97 @97AC97 I#0H)>PT*"0EC;W5T(#P\(")#871C:&EN
M9R!E>&-E<"!E>&-E<'1I;VXN(B \/"!E;F1L.PT*"0EC;W5T(#P\(")297%U
M97-T(&YO="!S871I<V9I960N(B \/"!E;F1L.PT*"7T-"@T*"6-O=70@/#P@
M(E9A;'5E(&]F('!#.B B(#P\('!#(#P\(&5N9&P[#0H-"@EC;W5T(#P\(")%
M;F1I;F<@<')O9W)A;2XB(#P\(&5N9&P[#0H-"@EC:&%R(&-C8SL-"@EC:6X@
,/CX@8V-C.PT*?0T*
`
end