Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 166626
Description
Marcin Garski
2005-08-23 23:28:34 UTC
Hi, Cool (I think). I'll take a look at it. If possible I would like to excahnge reviews as I myself also have a package which still needs review, see bug 165992 Thanks and I'll let you know if its ok or what needs changing. I assume you already have CVS access / are an FE contributer? First problem, on my system (rawhide) ./configure prints: xscorch 0.2.0 Configuration: Prefix: /usr Datadir: /usr/share Mandir: /usr/share/man CFLAGS: -O2 -g -pipe -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -m32 -march=i386 -mtune=pentium4 -fasynchronous-unwind-tables Sound: NO Network: Yes Readline: NO GTK 1.2: Yes GTK 2.0: NO GNOME: Yes Why no sound and no readline (you do BuildRequire Readline-devel) and why do you use gtk-1.2 (pass --with-gtk-12 to configure), thats so old and deprecated. xmms is about theonly program left in Fedora which still requires gtk12, and thats being rewritten to use gtk2. Anyways the build goes on, I'll see what else turns up. Well, I still haven't done a complete review instead I've been fixing things so that it will build against gtk2 and that it actually uses readline. See the attached spec and extra patch. I'll try todo a proper review tomorrow. Created attachment 118068 [details]
modfied spec file for building against gtk2, and which includes a configure patch for proper readline detection.
Created attachment 118069 [details]
Patrch file fixing the configure script to properly detect readline
This really is a readline bug (its .so is not linked to termcap), remind me to
file a bug about this if I don't add a comment about having filed a bug for
this soonish.
Erm, I tried running the (rawhide) compiled result (with my spec and patch) and it bombs out with the message: *** stack smashing detected ***: xscorch terminated Aborted This is a new feature in rawhide gcc which detects stack overruns. Can yuo debug this, otherwise I can help when I find the time. Also I should change the status of this bug, but I need to read what todo exactly at the wiki and don't have the time for that right now. (In reply to comment #5) > Created an attachment (id=118069) [edit] > Patrch file fixing the configure script to properly detect readline This patch probably can be avoided by appending -ltermcap to LIBS, i.e. %configure .... LIBS=-ltermcap probably will work OTB. As you can see (from Changelog): "Initiating conversion of interface code to GTK 2.0. The 2.0-specific code is NOT WELL TESTED yet. To force GTK 1.2, use the new configure option --with-gtk-12 (currently this is the default). To force GTK 2.0, use --without-gtk-12. Once GTK 2.0 support is stable, the default will change to using GTK 2.0 code." This is why I didn't enable GTK+ 2.x. Why not enabling sound (from configure): "*** NOTE *** You have enabled sound support, but currently there is no music, nor are there sound effects in the game. xscorch will be linked to mikmod but the sound options will currently do nothing. We are currently looking for volunteers to create music for xscorch. If you'd like to write us some tracks, please drop us an e-mail!" Of course I can enable it (what you already do in patched spec file). I'm also wonder about networking (from configure): "*** WARNING *** Network games are UNSTABLE. PLEASE read doc/NETWORK before sending in bug reports. Also, check out the document if you are interested in network development. We appreciate any feedback/bugfixes on the networking code." I've send email to xscorch developers about bugs that you have find, will see what they reply. In reply to comment #1. Hans yes I have access to CVS (since yesterday) :) I'll look at your package. Howdy all. :) Marcin is correct that there is no sound. Additionally, 0.2.0 does not use readline for anything yet, although we intend to use it in the server. Network games are indeed unstable, although they can be playable for a while. The default compile will create a useless binary called xscorch-server, which we forgot to suppress when we released -- the intent is to rewrite the networking entirely. Whether you turn networking on in your binary packages mostly depends on how many bug reports you want, I guess. :-/ Finally, GTK2 support *should* work, but there are at least two minor features we still haven't figured out how to implement in GTK2 yet, so you won't get them if you compile for it. Also, the GTK2 support is severely less tested (by us anyway). As for the stack overrun, it would help a lot if you could run gdb on it and get a stack trace... Also, testing to see if the bug is present when compiled against GTK1.2 would be useful. Finally finally, I don't know if this impacts FC or not but there is a mandatory patch for running 0.2.0 with 64-bit pointers. Marcin might have already applied it, though. It is here: http://www.xscorch.org/releases/xscorch-0.2.0-64bit.patch.gz comment 7: Yes that will probably work too, good idea. comment 8 & 10: gtk-1.2 is really ancient since gtk-2.0 support is ok although not thoroughly tested, I would prefer that we use this. Assuming the FE package gets used a lot, then gtk-2.0 support will get the needed testing :) About sound-support, yes I saw that too, I'm hoping the upstream developers will surprise us with a 0.3 release with sounds, better to already have the specfile ready then :) Aren't there any good free (as in freedom) modtracker download sites out there? comment 10: I'll take a look at the stackguard problem when i find the time, I know howto use gdb etc, I just didn't find the time yet. Also does you ./configure generate debugable binaries by default (CFLAGS does contain -g before starting configure) The 64 bit patch is already included in the spec. If you want debugging symbols you should give configure the --enable-debug flag, which I think is off by default. This may also get you some stdout stuff. In reply to comment 7: I tried using LIBS=-ltemrcap, does this make configure detect readline, but causes the make to fail because -ltermcap doesn't get added when linking. About the stack overflow, here is a (x86_64) backtrace: #0 0x00002aaaac17cb60 in raise () from /lib64/libc.so.6 #1 0x00002aaaac17e030 in abort () from /lib64/libc.so.6 #2 0x00002aaaac1b22a8 in __libc_message () from /lib64/libc.so.6 #3 0x00002aaaac22b8ef in __stack_chk_fail () from /lib64/libc.so.6 #4 0x0000000000415683 in _sc_scoring_read_item (ec=0x5ac4a0, reader=0x5ab760, item=0x5acda0) at saddconf.c:253 #5 0x0000000000415c71 in sc_addconf_append_file (type=Variable "type" is not available. ) at saddconf.c:461 #6 0x00000000004063ad in sc_economy_config_create (c=0x599010) at seconomy.c:66 #7 0x0000000000405bf4 in sc_config_new (argc=0x7fffff83757c, argv=0x7fffff837570) at sconfig.c:193 #8 0x00000000004058bf in main (argc=1, argv=0x7fffff837798) at xscorch.c:86 If I change: char desc[SC_INVENTORY_MAX_DESC_LEN]; char name[SC_INVENTORY_MAX_NAME_LEN]; to: char desc[SC_INVENTORY_MAX_DESC_LEN+256]; char name[SC_INVENTORY_MAX_NAME_LEN+256]; Things work fine, I can't find an obvious bufferoverflow in this functions or in functions to which desc/name get passed. sc_scoring_lookup_by_name does do ugly things (casting a ptr to a long int), but with that commented out the stacks still gets overflowed. p.s. I made the chanes to those 2 buffer declerations in _sc_scoring_read_item, the function which causes the stack overflow. Instead of +256, does it work safely with +1 already? Because this string in "weapons.def" is 80 chars plus terminating zero and hence overflows the local "desc" array: $ echo "We are in the final days. My time is come! Glory glory! What do you think? 5.1?" | wc -c 80 $ grep SC_INVENTORY_MAX_DESC_LEN * sgame/sinventory.h sgame/sinventory.h:#define SC_INVENTORY_MAX_DESC_LEN 80 I also tried with +1 to see if there were any off by one errors but that doesn't do the trick The code that reads those config files is supposed to truncate and null terminate (and is fairly well tested). I'd be surprised if it's not just removing the '?' at the end and replacing it with a null byte. It's unfortunate that this fancy new gcc stack stuff doesn't tell you where in the function the stack gets smashed. There is a lot going on in that function. :( Also, I'm curious about comment 13; what platforms are you aware of where long is not adequately wide to store a pointer in? If there are any such platforms which run redhat or debian then I need to change that code. long is fine to store a pointer in its just ugly and should be forbidden imho, thats all. Well, I saw at least one occurence of strncpy() and strlen() in the code that reads the description strings, and strncpy creates a non-null terminated array, if the null is at a position >= n. Those are strlenn and strncpyn actually, which are safe versions of the functions, defined in the sutil directory. They guarantee null termination. You can't link to the glibc versions of the string functions from in xscorch. Sorry, I was wrong. It's strcopyn and they're not in sutil anymore, they're in libj/jstr . Well, to be precise, it's strcopyb as strcopyn would need a buffer of size n+1. And strlenn is a preprocessor macro, STRLENN, which is just strlen(). But indeed, the STRNCPY macro inserts a null at pos n-1. You won't convince me to like such "C string" hacks, when a few lines below in saddconf.c you do manual size+1 calculations for malloc and strcopyn nevertheless. The normal C string implementation has a bias towards getting more data; ours has a bias towards null terminating strings. Either way you sooner or later end up doing math. *shrug* It's clear we have different preferences in coding style, and I'm not even remotely interested in arguing the topic. If we can track down the actual bug, that would be a great thing though. (In reply to comment #13) > In reply to comment 7: > I tried using LIBS=-ltemrcap, does this make configure detect readline, but > causes the make to fail because -ltermcap doesn't get added when linking. Yes, the configure.ac is broken and trashes LIBS Created attachment 118136 [details]
patch fixing stack smashing
I've found the problem (cut and paste error) causing the stack smashing, I've
attached a patch fixing this.
Created attachment 118137 [details] new specfile using stack-smash-fix.patch, remove network & sound support Here's the specfile I'm currently using for building, testing & as a base for my review. I've added the stack smashing fix and removed network support (unstable, will only get us bugreports and frustate users) and restored sound support to being disabled (only drags in unnescesarry deps). I've requested a reviewer role in the account system, this is waiting approval in the meanwhile I'll start walking through the list at: http://fedoraproject.org/wiki/PackageReviewGuidelines One more note about my specfile changes I also added: %dir %{_datadir}/xscorch But it seems that: %{_datadir}/xscorch/ does that automaticly, so that should be removed again. Also the changelog needs updating for the made changes. Reviewing: -rpmlint: Error while reading /home/hans/xscorch.spec It gives the same error while reading other specs appearantly rpmlint is broken, see bug 166826. skipping this step. -package name + specfile name: ok -packaging guidelines: ok -license + license tag + license in %doc: ok -spec file readable: ok -source matches upstream: ok -long list off small nitpicks: all ok So in short: -remove the %dir %{_datadir}/xscorch I added -write a changelog entry And its approved. New version on server: http://manta.univ.gda.pl/~mgarski/FE/xscorch-0.2.0-4.src.rpm http://manta.univ.gda.pl/~mgarski/FE/xscorch.spec I have add gtk2-devel >= 2.4.0 instead >= 2.6.0 because FC3 uses gtk2-devel-2.4.13 out-of-box, and comment BuildRequires: readline-devel because it's unnessesary requirement (as we don't enable networking). Looks good: APPROVED. Please import into CVS, build for devel. Request branches and build for those. after that this bug can be closed with a resolution of NEXTRELEASE. In the mean time can you asign the bug to me (j.w.r.degoede), for some unknown reason I can't reasign it to myself, even though I've added myself to fedorabugs in the account system. Thanks for finding that (comment 25); I might never have noticed it. The economy stuff has different display constraints so it's supposed to have different defines for them. I probably got interrupted halfway through the task. :-/ I've completed it and placed the patch on the xscorch.org web site. Created attachment 118183 [details] Version of upstream patch with leading-directories included I think it's better if we switch to upstream's version of the stack smashing patch, I've tested it and it works as advertised :) I've attached a version of this patch were the directories are included, so its a drop in replacement for my old patch. The original upstream didn't even include the sgame path in the patch file, so replacing -p1 with -p0 didn't do the trick and doing a cd sgame in the %prep is ugly. Jacob you may wish to replace your patch with mine, or atleast with one which includes the sgame dir, users are going to find it a pain to apply the one currently at xscorch.org. I've also taken a look at the needing -ltermcap for readline stuff. My patch for this is correct and will stay needed in the future see: bug 126023 for more info. Yup. That's what I get for being in a hurry to go camping... I've replaced the xscorch.org patch. Thanks. |