This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 166515 - Review Request: pbzip2 : parallel version of bzip2
Review Request: pbzip2 : parallel version of bzip2
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Oliver Falk
David Lawrence
http://compression.ca/pbzip2/
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-22 14:57 EDT by Jeff Gilchrist
Modified: 2015-09-14 09:46 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-09-07 12:21:54 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
limburgher: fedora‑cvs+


Attachments (Terms of Use)
Proposed patch to fix some pbzip2 issues (8.33 KB, patch)
2005-08-26 13:21 EDT, Jindrich Novy
no flags Details | Diff

  None (edit)
Description Jeff Gilchrist 2005-08-22 14:57:17 EDT
Spec Name or Url: http://compression.ca/pbzip2/pbzip2.spec
SRPM Name or Url: http://compression.ca/pbzip2/pbzip2-0.9.3-1.src.rpm
Description: 

Hello, a number of people have suggested that I submit pbzip2 into Fedora Extras so would greatly appreciate if people could review it.

PBZIP2 is a parallel implementation of the bzip2 block-sorting file compressor that uses pthreads and achieves near-linear speedup on SMP machines.  The output of this version is fully compatible with bzip2 v1.0.2 (ie: anything compressed with pbzip2 can be decompressed with bzip2).
Comment 1 Oliver Falk 2005-08-25 11:16:42 EDT
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.
Comment 2 Jindrich Novy 2005-08-25 13:34:14 EDT
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.
Comment 3 Jeff Gilchrist 2005-08-25 13:43:30 EDT
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.
Comment 4 Ville Skyttä 2005-08-25 13:58:29 EDT
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. 
Comment 5 Oliver Falk 2005-08-26 03:09:29 EDT
(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!

Comment 6 Oliver Falk 2005-08-26 03:10:44 EDT
Have you included the patch allready? As soon as you release a new version, let
me know, so I can review the package.
Comment 7 Jindrich Novy 2005-08-26 11:38:09 EDT
Hello Jeff, Oliver, I've been out for a while. I'll send the patch here this
evening.
Comment 8 Jindrich Novy 2005-08-26 13:21:32 EDT
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.
Comment 9 Jeff Gilchrist 2005-08-29 15:01:09 EDT
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.
Comment 10 Oliver Falk 2005-08-30 03:26:08 EDT
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. :-)
Comment 11 Jindrich Novy 2005-08-30 04:24:49 EDT
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 ;)
Comment 12 Jeff Gilchrist 2005-08-30 10:42:27 EDT
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.
Comment 13 Oliver Falk 2005-08-30 11:02:10 EDT
Arg. Yes, sorry, forgot, that it's only one file... It's OK of course...
Comment 14 Jeff Gilchrist 2005-08-30 12:59:10 EDT
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.
Comment 15 Warren Togami 2005-08-31 00:57:25 EDT
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.
Comment 16 Jeff Gilchrist 2005-08-31 08:58:26 EDT
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.
Comment 17 Jeff Gilchrist 2005-08-31 09:23:32 EDT
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.
Comment 18 Warren Togami 2005-08-31 13:56:22 EDT
Compressing data from stdin is important functionality.  I suspect your package
now may be ready for initial inclusion though.
Comment 19 Jeff Gilchrist 2005-09-02 09:48:50 EDT
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.
Comment 20 Jeff Gilchrist 2005-09-07 12:21:54 EDT
Added package to CVS and builds fine on devel, FC-3, and FC-4 according to CVS
build-system.
Comment 21 Adam Tkac 2015-09-14 06:44:47 EDT
Package Change Request
======================
Package Name: pbzip2
New Branches: epel7
Owners: atkac jeffg
Comment 22 Jon Ciesla 2015-09-14 09:46:05 EDT
Git done (by process-git-requests).

Note You need to log in before you can comment on or make changes to this bug.