Bug 192049
Summary: | Review Request: gnash - GNU Flash player | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Jens Petersen <petersen> |
Component: | Package Review | Assignee: | Patrice Dumas <pertusus> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | ch.nolte, chris.stone, dominik, gc, gnomeuser, laurent.rineau__fedora, maestronn, michael |
Target Milestone: | --- | ||
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
URL: | http://www.gnu.org/software/gnash/ | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-08-28 00:54:59 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 | ||
Attachments: |
Description
Jens Petersen
2006-05-17 01:37:27 UTC
* klash is now gnash-klash for the subpackages upstream, this name should be used here also for consistency with what will appear. * there is a security issue that should be patched in fedora extras package, indeed there is an insecure use of /tmp. If it is too much work, at least there should be a note somewhere. * the documentation should be distributed (see the specfile in the tarball for hints on how to do this), except if there is a good reason not to distribute it? At least manpage and html manual, info files and scrollkeeper files would be bonus * rpmlint gives those warnings: W: gnash devel-file-in-non-devel-package /usr/lib/libgnashasobjs.so W: gnash devel-file-in-non-devel-package /usr/lib/libgnashbackend.so W: gnash devel-file-in-non-devel-package /usr/lib/libgnashgeo.so W: gnash devel-file-in-non-devel-package /usr/lib/libgnashserver.so W: gnash devel-file-in-non-devel-package /usr/lib/libgnashbase.so Maybe it isn't necessary to have a distinct gnash-devel package, but in that case it may be good to Provides: %{name}-devel = %{version}-%{release} There is also this warning that may be problematic, although I don't know how to solve it: E: klash binary-or-shlib-defines-rpath /usr/lib/kde3/libklashpart.so ['/usr/lib', '/usr/lib/qt-3.3/lib'] Other rpmlint warnings/errors are not problematic. * Maybe the gnash package should be in Group: Applications/Multimedia and not in Applications/Internet (the plugins are rightly in Applications/Internet). * also you could have a look at the descriptions and summary in the spec file in the tarball and pick ideas, although it is up to you, the one you proposed are good. Thanks for the speedy review. :) Sorry I missed the upstream update again.... (In reply to comment #1) > * klash is now gnash-klash for the subpackages upstream, this name should > be used here also for consistency with what will appear. Ok. > * there is a security issue that should be patched in fedora extras package, > indeed there is an insecure use of /tmp. If it is too much work, at least > there should be a note somewhere. Is there a patch from cvs that can be backported for this? > * the documentation should be distributed (see the specfile in the tarball > for hints on how to do this), except if there is a good reason not to > distribute it? At least manpage and html manual, info files and > scrollkeeper files would be bonus Sounds good. I added buildrequires docbook2X for that. (In reply to comment #2) > W: gnash devel-file-in-non-devel-package /usr/lib/libgnashasobjs.so > W: gnash devel-file-in-non-devel-package /usr/lib/libgnashbackend.so > W: gnash devel-file-in-non-devel-package /usr/lib/libgnashgeo.so > W: gnash devel-file-in-non-devel-package /usr/lib/libgnashserver.so > W: gnash devel-file-in-non-devel-package /usr/lib/libgnashbase.so I removed them for now. > There is also this warning that may be problematic, although I don't > know how to solve it: > E: klash binary-or-shlib-defines-rpath /usr/lib/kde3/libklashpart.so > ['/usr/lib', '/usr/lib/qt-3.3/lib'] I added --disable-rpath to configure. > * Maybe the gnash package should be in > Group: Applications/Multimedia > and not in Applications/Internet (the plugins are rightly in Applications/Internet). Thanks, fixed. I also subpackaged the libraries. http://people.redhat.com/petersen/extras/gnash.spec SRPM URL: http://people.redhat.com/petersen/extras/gnash-0.7.1-2.src.rpm (In reply to comment #3) > > * there is a security issue that should be patched in fedora extras package, > > indeed there is an insecure use of /tmp. If it is too much work, at least > > there should be a note somewhere. > > Is there a patch from cvs that can be backported for this? Unfortunately not. I raised the issue on the gnash-dev list, though. That's the only thing that retained me from proposing gnash to FE ;-) > > * the documentation should be distributed (see the specfile in the tarball > > for hints on how to do this), except if there is a good reason not to > > distribute it? At least manpage and html manual, info files and > > scrollkeeper files would be bonus > > Sounds good. I added buildrequires docbook2X for that. This shouldn't be needed, as the files are allready up to date in the tarball. Doesn't hurt, though. There is a bug in doc/C/Makefile.am regarding info files installation during staged install. I'll attach patches. > > W: gnash devel-file-in-non-devel-package /usr/lib/libgnashbase.so > > I removed them for now. Agreed, this seems to be the best solution until they are really usefull to develop something. > I also subpackaged the libraries. Really? It doesn't seems so to me... Anyway I think it is better to have the libraries together with the standalone player. But if you want to split, you can; Created attachment 129322 [details]
use patch to skip install-info error
Created attachment 129323 [details]
don't stop on install-info errors during staged install
To build in mock in FC-5, I needed libXmu-devel. I don't know if it is really needed by gnash, it doesn't seems so to me, I asked upstream. (In reply to comment #4) > > I added buildrequires docbook2X for that. > > This shouldn't be needed, as the files are allready up to date in the > tarball. Doesn't hurt, though. Ok, configure tests for it anyway. > There is a bug in doc/C/Makefile.am regarding info files installation > during staged install. I'll attach patches. Oh, I didn't notice any error. > > I also subpackaged the libraries. > Anyway I think it is better to have > the libraries together with the standalone player. But if you want to > split, you can; Well the libs are required by each of the other subpackages so I thought it makes sense to separate them out: assuming many people would only want one of the plugins. (In reply to comment #8) > (In reply to comment #4) > > There is a bug in doc/C/Makefile.am regarding info files installation > > during staged install. I'll attach patches. > > Oh, I didn't notice any error. That's strange. Do you have /sbin in your path or are you building as root? Not a big deal, it is upstream now. > Well the libs are required by each of the other subpackages so I thought > it makes sense to separate them out: assuming many people would only > want one of the plugins. Indeed, but having the standalone player together with the plugin doesn't hurt and may even help, as sometimes the plugin fails but the standalone player work and the .swf is always downloaded. Once the plugins stream the flash maybe it could be reconsidered. (In reply to comment #8) > (In reply to comment #4) > > > I added buildrequires docbook2X for that. > > > > This shouldn't be needed, as the files are allready up to date in the > > tarball. Doesn't hurt, though. > > Ok, configure tests for it anyway. Yes, but when not found there is only a warning by configure, the make target still exist but the files are touch'ed instead of being regenerated. However since the output files are up to date they are left untouched and just copied. I added the info install patch, thanks. (Yes, I have sbin in my path...) I unsubpackaged the libs for now then. Since there is no desktop file yet either it probably doesn't matter too much. Perhaps when the plugin is more stable, subpackaging can be revisited as you suggest. I tried removing the docbook2X buildreq, but then had to remove --enable-docbook too to stop configure barfing, and then the manpage, info manual and omf file didn't get installed... http://people.redhat.com/petersen/extras/gnash.spec http://people.redhat.com/petersen/extras/gnash-0.7.1-3.src.rpm This package builds ok for me with mock under fc5. Created attachment 131128 [details]
Standard error of rpmbuild
It does not compile on my x86_64. Here you are my configuration Linux lampone 2.6.16-1.2129_FC5 #1 SMP Thu Jun 1 10:59:33 EDT 2006 x86_64 x86_64 x86_64 GNU/Linux libjpeg-devel-6b-36.2.1 libogg-devel-1.1.3-1.2 libxml2-devel-2.6.23-1.2 SDL_mixer-devel-1.2.6-7.fc5 kdelibs-devel-3.5.3-0.2.fc5 gtkglext-devel-1.2.0-2.fc5 docbook2X-0.8.7-1.fc5 scrollkeeper-0.3.14-5.2.1 The errors when I try to execute rpmbuild --rebuild gnash-0.7.1-3.src.rpm are attached in the previous post P.S. Is is possible to exclude compilation of knash? BTW, if I modify the spec file by replacing --disable-klash for --enable-klash and removing all the rest of the stuff about klash then it compiles (In reply to comment #13) > It does not compile on my x86_64. It seems like the qt headers or libs aren't found by ./configure. Do you have qt-devel installed? It should be required by kdelibs-devel which is installed. Could you also please post the config.log file which is in the rpm build directory. The build.log reference in comment #12 contains: ... g++ -DHAVE_CONFIG_H -I. -I. -I../.. -I.. -I../.. -I../../server -I../../libbase -I../../backend -I../../libgeometry -I/usr/include -I/usr/include/SDL -I/usr/include/SDL -I/usr/include/libxml2 -I/usr/include/kde/kio -I/usr/include/kde -I. -DQT_THREAD_SUPPORT -D_REENTRANT -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wall -c klash_part.cpp -fPIC -DPIC -o .libs/klash_part.o klash_part.cpp:45:22: error: qcstring.h: No such file or directory klash_part.cpp:46:24: error: qpopupmenu.h: No such file or directory klash_part.cpp:47:20: error: qtimer.h: No such file or directory In file included from klash_part.cpp:50: /usr/include/kde/klibloader.h:21:21: error: qobject.h: No such file or directory /usr/include/kde/klibloader.h:22:21: error: qstring.h: No such file or directory /usr/include/kde/klibloader.h:23:25: error: qstringlist.h: No such file or directory /usr/include/kde/klibloader.h:24:24: error: qasciidict.h: No such file or directory /usr/include/kde/klibloader.h:25:22: error: qptrlist.h: No such file or directory So, clearly qt's headers aren't being found. The compiler step *should* include -I${QTINC} where the QTINC environment variable expands to %_libdir/qt-3.3/include > It seems like the qt headers or libs aren't found by ./configure. > Do you have qt-devel installed? It should be required by > kdelibs-devel which is installed. Yes I do: qt-devel-3.3.6-0.1.fc5 > Could you also please post the config.log file which is in > the rpm build directory. Uhuuu, that really looks crazy. In order to generate the confi.log I just executed again rpmbuild --rebuild gnash-0.7.1-3.src.rpm . Guess what? Now it compiles. WHich is completely absurd since I did nothing in the meanwhile. Not even rebooted the machine. The only thing I did between the two compilations, is to modify the spec compile clash and install the obtained rpms. But I carefully cleaned up /usr/src/redhat before recompiling. Well, now I have my /usr/src/redhat/RPMS/x86_64/gnash-klash-0.7.1-3.x86_64.rpm Wonderful, after 21 years as a computer scientist these boxes can still surprise me :-) Fortunately my attachment proves that I'm not completely crazy (yet) Well, so long ... Giuseppe, you may have just lucked into the fact that the latest, recently-released qt-3.3.6-0.1 contains the fix for this particular problem, reported as bug #169132 This fails to build in mock. You need to add: BuildRequires: autoconf, automake, libtool Thanks. Should be fixed in: http://people.redhat.com/petersen/extras/gnash.spec http://people.redhat.com/petersen/extras/gnash-0.7.1-4.src.rpm I think the only remaining issue is Patrice's concerns about a potential tmp file vulnerability in the mozilla plugin (it just downloads .swf files straight to /tmp), though that is really an upstream issue. I'm not sure how the current behaviour compares with other plugins but probably a reasonable soltion is to use mkdtemp in /tmp. Created attachment 133223 [details]
plugin-tempfile-dir.patch
untested patch to use mkdtemp
Created attachment 133235 [details]
plugin-tempfile-dir.patch
better patch still untested
I'll test the patch but it would benefit from a bit of error code testing ;-) It segfaults with this patch. My wild guesses: the template is constant, although, quoting the manpage: "Since it will be modified, template must not be a string constant, but should be declared as a character array." The other possibility I see is that something is needed to convert the resulting char* to string. Same problem with QT on my amd64 box, too. Sourcing /etc/profile.d/qt.sh and adding --with-qtdir=$QTDIR to %configure helps. Another problem: it crashes the whole GNOME (i.e panel and terminal) on my FC5 AMD64 box when you enter the following URL (so don't click it blindly!): B I G F A T W A R N I N G THIS WILL KILL YOUR GNOME SESSION DON'T SAY YOU WEREN'T WARNED! http://www.cafepress.com/cp/search/search.aspx?cfpt=118%3A________________H&q=B5&x=0&y=0&cfpt2=&copt=&source=searchBox (In reply to comment #26) > Another problem: it crashes the whole GNOME (i.e panel and terminal) on my FC5 > AMD64 box when you enter the following URL (so don't click it blindly!): That doesn't look like a gnash bug, but a GNOME (or maybe Xorg/Mesa) bug triggered by gnash. gnash embedded in firefox shouldn't be able to crash anything else than firefox. I also had many bugs revealed by gnash on FC5. Things are smoother on devel (but with other issues, like Xorg not starting with latest update of the driver, it's rawhide after all ;-) The klash plugin fail to build on x86_64 arch using uptodate rawhide. Are there additional build-requirements? Thanks, problems in comment 24 and comment 25 (comment 28) should be fixed in: http://people.redhat.com/petersen/extras/gnash.spec http://people.redhat.com/petersen/extras/gnash-0.7.1-5.src.rpm I reproduced comment 26: this no longer seems to happen with gnash cvs head fwiw. Great! I'm really looking forward to having this in extras. Browsing to sites with flash 9 causes my X session/server to restart on my x86_64 running rawhide. I'm not sure if it's because of flash 9 but it happens on sites like gap.com & myspace. (In reply to comment #31) > Browsing to sites with flash 9 causes my X session/server to restart on my > x86_64 running rawhide. This sounds like the problem in comment 26, but I agree this blocks acceptance. As I noted in comment 29 it seems to be fixed in cvs. (In reply to comment #32) > (In reply to comment #31) > > Browsing to sites with flash 9 causes my X session/server to restart on my > > x86_64 running rawhide. > > This sounds like the problem in comment 26, but I agree this blocks acceptance. > As I noted in comment 29 it seems to be fixed in cvs. Once again it is not a gnash bug, but certainly an xorg/mesa bug triggered by gnash. The patch works for me. It fills /tmp with temporary dirs, but it is better than what was before. It should be documented, however. So I propose to add a README.fedora in the gnash-plugin documentation. I attach a spec diff and a gnash-README.fedora. I also think that it would be better to prefix plugin-tempfile-dir.patch with gnash, such that it is called gnash-plugin-tempfile-dir.patch instead. Created attachment 134526 [details]
explanation of the /tmp/gnash-XXXXXX files
Created attachment 134527 [details]
spec file diff to install the README for the plugin
Another remark, autoconf is required by automake. (In reply to comment #33) > Once again it is not a gnash bug, but certainly an xorg/mesa bug > triggered by gnash. Nod, but until that is fixed it doesn't make sense really to include gnash in Extras. I suggest a bug be opened for that issue and that it block this bug. (In reply to comment #34) > The patch works for me. It fills /tmp with temporary dirs, but > it is better than what was before. It should be documented, however. > So I propose to add a > README.fedora in the gnash-plugin documentation. I attach a spec > diff and a gnash-README.fedora. Ok. > I also think that it would be better to prefix plugin-tempfile-dir.patch > with gnash, such that it is called gnash-plugin-tempfile-dir.patch > instead. Why? :) (In reply to comment #37) > Another remark, autoconf is required by automake. So you mean it shouldn't be in BR? It can be removed I suppose though it makes the dependency on autoreconf less obvious... Perhaps autoconf should require automake too? (In reply to comment #38) > Nod, but until that is fixed it doesn't make sense really to include gnash in > Extras. I suggest a bug be opened for that issue and that it block this bug. This bug will likely be driver dependent... > > I also think that it would be better to prefix plugin-tempfile-dir.patch > > with gnash, such that it is called gnash-plugin-tempfile-dir.patch > > instead. > > Why? :) Because it helps knowing that it is a source file associated with the gnash rpm. Especially handy when you have a lot of patches and source in SOURCES. But it is not a blocker, just a remark. > (In reply to comment #37) > > Another remark, autoconf is required by automake. > > So you mean it shouldn't be in BR? It can be removed I suppose > though it makes the dependency on autoreconf less obvious... > Perhaps autoconf should require automake too? autoconf shouldn't require automake, since it doesn't require automake. In our case builrequires for autoconf is not that bad, it is just an unneeded buildrequires, and the practice (and I think it is somewhere in the guidelines) is to avoid buildrequires when there are allready implied by another package. Not a blocker (other reviewers would consider that a blocker, I think) (In reply to comment #39) > > I suggest a bug be opened for that issue and that it block this bug. > > This bug will likely be driver dependent... Quite possible. Perhaps better would be to update the gnash source to cvs head? > Because it helps knowing that it is a source file associated with > the gnash rpm. Especially handy when you have a lot of patches and > source in SOURCES. Ok, you're right of course. It is so long that I've used the default directories for rpmbuilding, that I had quite forgotten about this namespace issue. (Personally I think it is much saner to build packages from separate directories...) > autoconf shouldn't require automake, since it doesn't require automake. (but autoreconf does) > In our case builrequires for autoconf is not that bad, it is just an > unneeded buildrequires, and the practice (and I think it is somewhere > in the guidelines) is to avoid buildrequires when there are allready > implied by another package. Not a blocker (other reviewers would consider > that a blocker, I think) I'll remove it anyway. Updated package: http://people.redhat.com/petersen/extras/gnash.spec http://people.redhat.com/petersen/extras/gnash-0.7.1-5.src.rpm For the record I don't really like the "flooding" of tmpdirs behaviour very much, but it seems like the simplest secure implementation possible. I guess X uses something similar for its /tmp/xses-$USER.XXXXXX session log files. A better implementation would probably save the .swf files in a directory like "/tmp/gnash-$USER/" owned by USER having permission 0700. It should also take account of TMPDIR I suppose. But I'm lazy... ;) (In reply to comment #40) > Quite possible. Perhaps better would be to update the gnash source to cvs head? That would be bad idea, since the cvs head is moving very fast. In my case what fixed the X crash issues was not a change in gnash, but switching to the rawhide Mesa/xorg (but not to the latest drv-i810...) > Updated package: > http://people.redhat.com/petersen/extras/gnash.spec > http://people.redhat.com/petersen/extras/gnash-0.7.1-5.src.rpm > > For the record I don't really like the "flooding" of tmpdirs behaviour very > much, but it seems like the simplest secure implementation possible. I guess > X uses something similar for its /tmp/xses-$USER.XXXXXX session log files. > A better implementation would probably save the .swf files in a directory like > "/tmp/gnash-$USER/" owned by USER having permission 0700. > It should also take account of TMPDIR I suppose. But I'm lazy... ;) It also seems the best compromise to me. It won't be a problem in next release anyway, with streaming, and personal directory. All the issues have been solved, so I am ready to approve. One last thing I would like is that somewhere it is marked that gnash is usable, but also that it won't work for many flash sites and that it is known to crash often and even trigger bugs that crash the graphical system (or something along those lines), either in the README.fedora or, maybe better, in the description. There is a missing %defattr(-,root,root,-) for klash Patrice, good points: would you like to take ownership of this package? :-) Created attachment 134912 [details]
spec file patch to add a warning in description and fix deffatr for klash
Sure, but it will be simpler if I take ownership after it is imported in cvs by you. I attach a patch for the spec file, with that patch applied it is approved, you can then import it in cvs, but add me in the owner.list as primary owner, and put yourself in the initial cc list. Does it sounds good? Another thing, I don't think there shouldn't be a branch for FC-5, given that gnash seems to trigger a mesa bug a bit too often, which seems to be fixed in FC-6. Does that sound good? Yup, sounds good to me. I will import it over the weekend. I imported gnash-0.7.1-7 into cvs. and added entry in owners.list too. I think this can be moved to FE-ACCEPT now, thanks. Patrice, please feel free to go ahead and build this. (In reply to comment #46) > Another thing, I don't think there shouldn't be a branch for > FC-5, given that gnash seems to trigger a mesa bug a bit too often Perhaps an FC-5 branch can be made if and when such a bug is resolved for fc5. I don't have FC5 to test on. I'll try to have a look from time to time, to see if mesa is updated to mesa-libGL-6.5.* and otherwise wait for user asking a branch. This is built now in devel, Jens you can close the bug. |