First climpse i took:
Good:
  * Group Applications/File is the same as bzip2 and therefor a good choice
  * License BSD, is OK. The README says the same
  * tmppath OK
  * Consitant use of tags
  * changelog ok

Bad:
 * Remove Vendor/Packager Tags
 * Use rm -rf %{buildroot}, and forget the [ ... ] check
 * Remove the cleanup of the buildroot in the %prep-section
 * Run rpmlint:
   - W: pbzip2 strange-permission pbzip2.spec 0600
 * Builds find
 * Installs/Uninstalls fine
 * Owns all files it installs.
 * Works. :-)

Notes:
 * You can drop bzip2-libs from requires, as RPM will detect libbz2.so.1 as
requirement.


If you fix the above stuff, I'll review again. If it's OK, I'll approve it.

From a brief look into the code:

<nitpick>
bzerr_dummy variable is unused in pbzip2.cpp:1191
</nitpick>

Another thing in pbzip2.cpp:

ret = read(hInfile, tmpBuff, strlen(bz2Header)+1);
close(hInfile);
if ((ret == -1) || (ret < strlen(bz2Header)+1))

Note that 'ret' variable is int but should be of size_t type so I'd use
some other than 'ret' variable here: (bzip2.c:1651-1654)

size_t size;
...
size = read(hInfile, tmpBuff, strlen(bz2Header)+1);
close(hInfile);
if ((size == (size_t)(-1)) || (size < strlen(bz2Header)+1))

numBlocks in pbzip2.cpp:1800 should be better of off_t data type.
BTW. why do you define uint_special and don't let it off_t at all?

Suggest getting rid of the redundant modulo and condition:

if ((fileSize % blockSize) == 0)
        numBlocks = fileSize / blockSize;
else
        numBlocks = fileSize / blockSize + 1;

by:
numBlocks = (fileSize + blockSize - 1) / blockSize;

Note that you don't previously check that blockSize != 0,
So that pbzip2 -b0 <anyfile> ends up with Floating point exception!!

Needs further fixes. I'll send you a patch to fix these and other issues I found.

New Spec: http://compression.ca/pbzip2/pbzip2.spec

I changed the .spec file to include Oliver's suggestions but I can't get rid of
the rpmlint strange-permission warning.

The pbzip2.spec file is 0664 in the tarbail inside the SRPM file.  If I change
that to 0644 or even 0444 I still get the same warning of strange-permission 0600.

Any idea how to fix this?  I will generate a new SRPM when I get the patch from
Jindrich.

As long as any SourceX or the specfile are not executable, ignore the 
strange-permission warning.  They'll change after importing to CVS anyway. 
 
FWIW, I believe rpmlint is complaining about the specfile you use to build the 
package, not one inside a tarball. 

(In reply to comment #4)
> As long as any SourceX or the specfile are not executable, ignore the 
> strange-permission warning.  They'll change after importing to CVS anyway. 

Yes, it will. It wasn't exactly this error that I was complaining about, I just
wanted to note, that running rpmlint on the srpm is a good idea. :-)

> FWIW, I believe rpmlint is complaining about the specfile you use to build the 
> package, not one inside a tarball.

Yes, that's correct!

Have you included the patch allready? As soon as you release a new version, let
me know, so I can review the package.

Hello Jeff, Oliver, I've been out for a while. I'll send the patch here this
evening.

Created attachment 118168 [details]
Proposed patch to fix some pbzip2 issues

The patch removes the bogus uint_special data type and replaces it by off_t. It
fixes stack based buffer overflow by specifying too long number in
-p<processors> parameter and restricts it to maximum number of 4096 supported
processors to prevent allocation failures with big numbers. Further it
reimplements -b<blocksize> comandline parsing to prevent another buffer
overflow and division by zero.

Few suggestions: use getopt() for commandline parsing, add error checks to make
pbzip2 more secure. The current implementation will require rewrite from
scratch of several parts of the code to become stable.

NEW Spec: http://compression.ca/pbzip2/pbzip2.spec
NEW SRPM: http://compression.ca/pbzip2/pbzip2-0.9.4-1.src.rpm

Modified spec file as per Oliver's suggestions.  Applied patch from Jindrich and
fixed another bug I found while I was at it (hence version # increase).

Can you please verify this new package?  Thanks.

Make is better written as: make %{?_smp_mflags}, if parallel build works.
If smp builds don't work it's approved this way, else add the smp_mflags and
it's also approved. :-)

Jeff, it looks much better now. However, I noticed yet another denial of service
in pbzip2:

When specifying a number of processors on commandline by -p<number>, pbzip2
allows an user to specify -p0, what tells pbzip2 to use no processor. This leads
to hang of pbzip2 and no file is compressed. Maybe this should be fixed in the
next pbzip2 release.

It's ok with me to include this release of pbzip2 into Extras as it makes life of 
many people easier ;)

Oliver:  There is only 1 file to compile so adding the SMP flags wouldn't get
you anything speed-wise.

Jindrich:  Good find, I haven't publicly released 0.9.4 yet so I will quickly
fix that and put up a new package.  Of course someone would have to go out of
their way to denial of service themselves and I'm sure they would notice the
problem and correct but its better to avoid that in the first place.

New package coming ASAP.

Arg. Yes, sorry, forgot, that it's only one file... It's OK of course...

NEW Spec: http://compression.ca/pbzip2/pbzip2.spec
NEW SRPM: http://compression.ca/pbzip2/pbzip2-0.9.4-1.src.rpm

Ok hopefully this is the last one.  New spec and SRPM built including a fix for
the -p0 bug that Jindrich found.

I didn't try your software, but from the website it looks like the default
output is very verbose.  Wouldn't it be better to be silent by default like
bzip2 with an optional verbose mode?  That way it can be used as a drop-in
replacement in scripts and people wouldn't notice any difference.

Warren, having the default mode be silent like bzip2 is probably a good idea. 
Since I have been testing it so much lately I probably kept the default verbose
to save myself from typing -v on the command line every time.  ;-)  Its not
quite ready for full drop-in replacement since it does not yet support
compressing data from stdin.  That is still on my todo list.  I will update the
SRPM so it is silent by default and let everyone know when it is ready.

NEW Spec: http://compression.ca/pbzip2/pbzip2.spec
NEW SRPM: http://compression.ca/pbzip2/pbzip2-0.9.4-2.src.rpm

Updated SRPM to make pbzip2 silent by default, now requires -v to make verbose.

PS: I might have set the status to NEEDINFO from NEW by mistake.

Compressing data from stdin is important functionality.  I suspect your package
now may be ready for initial inclusion though.

Does that mean its going to be accepted?  If yes, then I will need CVS access to
submit the software.  I have already completed the legal papers and my CVS user
id is: jeffg

Thanks,
Jeff.

Added package to CVS and builds fine on devel, FC-3, and FC-4 according to CVS
build-system.

Package Change Request
======================
Package Name: pbzip2
New Branches: epel7
Owners: atkac jeffg

Git done (by process-git-requests).