*** Bug 198098 has been marked as a duplicate of this bug. ***

* The LEGGIMI file should certainly be tagged with:
%lang(it) %doc LEGGIMI

* I think that the icon should also be in 
/usr/share/icons/hicolor/48x48/apps/ since it is specific of the 
size (I guess it should also be kept in /usr/share/pixmap for the
about).

Thanks for the comments, both fixed in
SPEC:
http://home.arcor.de/christoph.wickert/fedora/extras/review/SPECS/xarchiver.spec
SRPM:
http://home.arcor.de/christoph.wickert/fedora/extras/review/SRPMS/xarchiver-0.4.2-0.3.rc2.fc7.src.rpm

More thoughts? Have you thought about my suggestion in bug #215241 comment #10?

Unused dependency on sonames. I haven't investigated where they 
come from, most probable is .pc file not using correctly *.private
(libm and libdl are certainly not problematic):

        /usr/lib/libatk-1.0.so.0
        /lib/libm.so.6
        /usr/lib/libpangocairo-1.0.so.0
        /usr/lib/libpango-1.0.so.0
        /usr/lib/libcairo.so.2
        /lib/libgmodule-2.0.so.0
        /lib/libdl.so.2

There is a requires for binutils, for ar, for .deb. Also reading
src/deb.c, it seems to me that the files in /tmp are not safely
created, opening the possibility of a symlink in tmp attack. Maybe
the manipulation should be done in a /tmp subdir. I haven't checked
the other /tmp use, some look clearly right, for others there is
a need to look at the code.

in src/callback.c, in xa_activate_link, I think it would be 
relevant to add a search from htmlview, and add a Requires for
htmlview. Otherwise a Requires firefox could be used, but I think 
it would be much better to call htmlview.

There is also a missing requires of cpio for rpm.

Thanks for catching these. I added the missing Requires to the specfile. Also
updated desktop-file-install to add more mime-types (application/x-ar,  
application/x-cd-image, application/x-deb, application/x-rpm, ...) to
xarchiver.desktop.

Yesterday I updated to 0.4.4, today I found 0.4.6 released. Fixes the icon issue
from comment #2, no changes regarding the unused direct dependencies. Most of
the xfce pacakges are really bad in this.

SPEC:
http://home.arcor.de/christoph.wickert/fedora/extras/review/SPECS/xarchiver.spec
SRPM:
http://home.arcor.de/christoph.wickert/fedora/extras/review/SRPMS/xarchiver-0.4.6-1.fc7.src.rpm

I will contact upstream about the deb thing.

I don't think the x-lha and x-lhz should be in mime in 
.desktop file since lha is not in fedora. I think it is the 
same for x-rar. I also don't think that deb and rpm should 
be included in the mime handled by xarchiver since it 
doesn't really seems to handle rpm or deb as rpm or deb but 
unarchive them. x-ar seems wrong to me too.

Regarding htmlview, a patch is also needed, currently it is
not used in the code!

(In reply to comment #6)
> I don't think the x-lha and x-lhz should be in mime in 
> .desktop file since lha is not in fedora. I think it is the 
> same for x-rar. I also don't think that deb and rpm should 
> be included in the mime handled by xarchiver since it 
> doesn't really seems to handle rpm or deb as rpm or deb but 
> unarchive them. x-ar seems wrong to me too.
 
All of these mimetypes are included in file-roller and/or ark packages from
Core, too. And why not adding xarchiver for rpms and debs? file-roller or ark
also can't do more then list/extact debs/rpms, noone expects an achive manager
to install packages. I use file-roller a lot to quickly look into rpms, get the
spec from an srpm etc. The default action still is system-install-packages, so
IMO adding xarchiver can't do no harm.

> Regarding htmlview, a patch is also needed, currently it is
> not used in the code!

I created a small patch for htmlview, also added epiphany, konqueror and seamonkey.

http://home.arcor.de/christoph.wickert/fedora/extras/review/SPECS/xarchiver.spec
http://home.arcor.de/christoph.wickert/fedora/extras/review/SRPMS/xarchiver-0.4.6-2.fc7.src.rpm

(In reply to comment #7)

> All of these mimetypes are included in file-roller and/or ark packages from
> Core, too. 

That's not a good reason. Since we don't ship lha and rar we shouldn't
open the corresponding files. file-roller and ark are broken. Also remember
that core packages haven't gone through review and may contain mistakes.
This one is not an obvious mistake, it may have been done on purpose, 
but still it is a mistake in my opinion.

x-ar isn't in the mimetype database. x-archive is, but since xarchive
fails to open ar archives, I am not sure that it is right.

> And why not adding xarchiver for rpms and debs? file-roller or ark
> also can't do more then list/extact debs/rpms, noone expects an achive manager
> to install packages. I use file-roller a lot to quickly look into rpms, get the
> spec from an srpm etc. The default action still is system-install-packages, so
> IMO adding xarchiver can't do no harm.

Ok for rpm and debs, the system of default seems right to me. And
if one don't have the default app (pirut for rpm) installed, why 
not xarchiver.

I get a segfault with .a. It seems that it is detected as
a .deb?

(gdb) run
Starting program: /usr/bin/xarchiver libz.a
[Thread debugging using libthread_db enabled]
[New Thread -1209128752 (LWP 2679)]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1209128752 (LWP 2679)]
0x00c971eb in strlen () from /lib/libc.so.6
(gdb) bt
#0  0x00c971eb in strlen () from /lib/libc.so.6
#1  0x077a4729 in g_strconcat () from /lib/libglib-2.0.so.0
#2  0x0805b91d in OpenDeb (archive=0xa1ddb48) at deb.c:32
#3  0x080580b7 in xa_open_archive (menuitem=0x0, data=0xa1ddb28)
    at callbacks.c:387
#4  0x0804f78d in main (argc=Cannot access memory at address 0x1
) at main.c:235
#5  0x00c40e5c in __libc_start_main (main=0x804efb0 <main>, argc=2, 
    ubp_av=0xbfa3d2a4, init=0x806a470 <__libc_csu_init>, 
    fini=0x806a460 <__libc_csu_fini>, rtld_fini=0x727490 <_dl_fini>, 
    stack_end=0xbfa3d29c) at libc-start.c:222
#6  0x0804df41 in _start ()
(gdb) 

After a quick reading of the code, the .a is indeed considered
to be a debian archive because it begins with !<arch> like .deb.
It should be better to test for !<arch>\ndebian (tests in 
callback.c line 1195), like libmagic does. But the segfault 
should be investigated prior.

Maybe upstream should better use libmagic to detect file type?
Also in /usr/share/file/magic one can see that there are more
than one kind of debian archive. Seems like file-roller and ark 
don't use libmagic, though.

Still on the subject of ar archives, ark and file-roller
seems to handle them correctly, maybe it could be an 
interesting feature for xarchiver?

Hey Patrice. Are you going to review this package? 
If not, I would be happy to do so, but it looks like you have already looked it
over quite a bit... ;) 

I am indeed reviewing it, but I don't assign it to me
until I am ready to approve, because there is a disagreament
about the mimetypes, so if somebody disagrees with me on
that subject, I don't want to stop him from reviewing the
package. To summarize my position

* I don't want to accept the package with the mimetypes
  for lha and rar without further discussion
* the segfault with the .a is not a blocker to me for
  devel, but it is for other branches (it may open a 
  security risk).
* otherwise the package is right. #comment 10 is not
  blocking, it is more like suggestions for upstream.

Sorry for slowing this down...

(In reply to comment #8)
> 
> x-ar isn't in the mimetype database. x-archive is, but since xarchive
> fails to open ar archives, I am not sure that it is right.

Of x-ar was wrong. My fault, if I did not add this mimetype most likely nobody
would have tried to open an .a file ;) Also I should have tested it before.

(In reply to comment #13)
> 
> * I don't want to accept the package with the mimetypes
>   for lha and rar without further discussion

As I have not tested lha I have also removed x-lha and x-lhz now. I still would
like rar in. If required programm (from the other repo) is not installed,
xarchiver will show a message that tells you to install it. This should IMHO be
allowed.

> * the segfault with the .a is not a blocker to me for
>   devel, but it is for other branches (it may open a 
>   security risk).

Can you please clairfy the symlink-attack problem from comment #4 a little? My
hacking skills are too low to explain Guiseppe what you mean. Seems like he
already noticed there's something not sane, see
http://bugzilla.xfce.org/show_bug.cgi?id=2616

(In reply to comment #14)

> As I have not tested lha I have also removed x-lha and x-lhz now. I still would
> like rar in. If required programm (from the other repo) is not installed,
> xarchiver will show a message that tells you to install it. This should IMHO be
> allowed.

Ok for rar.

> Can you please clairfy the symlink-attack problem from comment #4 a little? My
> hacking skills are too low to explain Guiseppe what you mean. Seems like he
> already noticed there's something not sane, see
> http://bugzilla.xfce.org/show_bug.cgi?id=2616

If a program creates a file below /tmp with a predictable name,
it opens a possibility for this well known attack. In short an attacker
have to create the conditions for a race condition by slowing down
xarchiver, then creates a symlink in /tmp which overwrites a file. 

A longer story is: the attacker waits for you to begin opening a .deb,
slows xarchiver, create a symlink in /tmp/ with the predictable name
pointing to one of your file, and this file content will be 
overwritten by the newly created file content. A simple fix is to 
use mkdtemp or mkstemp to create the directory or the file with an
unpredictable name.

The opening of ar archives has been fixed in SVN r24084.

What's left to do?

I'll have to package the svn version. I'm not really sure that the segfault is
really fixed.

Update to 0.4.9-0.1.svn_r24096:

- fixes segfault from comment #9
- xa now checks for debs correctly (comment #10)
- remove all patches, included upstream now
- remove mimetypes x-ar, x-lha and lx-hz
- remove mimetype x-deb as long as debs are extracted to /tmp/data.tar.gz

SPEC:
http://home.arcor.de/christoph.wickert/fedora/extras/review/SPECS/xarchiver.spec
SRPM:
http://home.arcor.de/christoph.wickert/fedora/extras/review/SRPMS/xarchiver-0.4.9-0.1.20061213svn.fc6.src.rpm

In the spec file comment, there is
svn co http://svn.xfce.org/svn/goodies/xfce4-websearch-plugin/trunk
xfce4-websearch-plugin

I think it should be
svn co -r24096 http://svn.xfce.org/svn/xfce/xarchiver/trunk xarchiver

Otherwise

* rpmlint gives:
W: xarchiver mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 12)
* name is right
* follow guidelines
* svn snapshot used for good reasons
* .desktop file shipped
* icons installed scriptlets used correctly
* match upstream (verified with a diff)
* %files section right

needs work:
Should the BR be gettext or gettext-devel? Currently it seems
to need some autoconf macros from gettext-devel, but even after
they are not needed anymore isn't gettext-devel needed?

APPROVED, with the gettext question answered, and the proper comment
for source generation added.

Please, don't push to FC-6 or FC-5 until the security issue has been
solved.

(In reply to comment #19)
> In the spec file comment, there is
> svn co http://svn.xfce.org/svn/goodies/xfce4-websearch-plugin/trunk
> xfce4-websearch-plugin

Oops, thanks for catching this. 

> Should the BR be gettext or gettext-devel? Currently it seems
> to need some autoconf macros from gettext-devel, but even after
> they are not needed anymore isn't gettext-devel needed?

Should be gettext for %find_lang. Because this package is a SVN snapshot it also
BuildRequires xfce-dev-tools, which have a dependency on gettext-devel (I think
you know, because you reviewed it)

> APPROVED, with the gettext question answered, and the proper comment
> for source generation added.

Ok, will do that tomorrow, now I'm going to bed. I promise not to publish this
for Core 5/6 until http://bugzilla.xfce.org/show_bug.cgi?id=2616 has been resolved.

(In reply to comment #20)

> Should be gettext for %find_lang. Because this package is a SVN snapshot it also
> BuildRequires xfce-dev-tools, which have a dependency on gettext-devel (I think
> you know, because you reviewed it)

Right.

23856 (xarchiver): Build on target fedora-development-extras succeeded.
     Build logs may be found at
http://buildsys.fedoraproject.org/logs/fedora-development-extras/23856-xarchiver-0.4.9-0.1.20061213svn.fc7/

Closing NEXTRELEASE

Bug http://bugzilla.xfce.org/show_bug.cgi?id=2616 has been resolved. So when
will xarchiver land on Fedora Extras for FC5/6?

Please give me another two days. I'll need some more time to investigate the
solution.

Extracting debs now is safe, the file name no longer is predictable. CVS Sync is
requested, archiver and thunar-archive-plugin should be in the repo soon.

Package Change Request
======================
Package Name: xarchiver
New Branches: EL-4 EL-5
Owners: cwickert

cvs done.