Bug 166626 - Review Request: xscorch - A Scorched Earth clone
Review Request: xscorch - A Scorched Earth clone
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
David Lawrence
http://manta.univ.gda.pl/~mgarski/FE/
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-08-23 19:28 EDT by Marcin Garski
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-08-28 06:14:56 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
modfied spec file for building against gtk2, and which includes a configure patch for proper readline detection. (2.16 KB, text/plain)
2005-08-24 11:01 EDT, Hans de Goede
no flags Details
Patrch file fixing the configure script to properly detect readline (1.41 KB, patch)
2005-08-24 11:03 EDT, Hans de Goede
no flags Details | Diff
patch fixing stack smashing (516 bytes, patch)
2005-08-26 03:33 EDT, Hans de Goede
no flags Details | Diff
new specfile using stack-smash-fix.patch, remove network & sound support (2.27 KB, text/plain)
2005-08-26 03:37 EDT, Hans de Goede
no flags Details
Version of upstream patch with leading-directories included (1.78 KB, patch)
2005-08-27 03:14 EDT, Hans de Goede
no flags Details | Diff

  None (edit)
Description Marcin Garski 2005-08-23 19:28:34 EDT
Spec Name or Url: http://manta.univ.gda.pl/~mgarski/FE/xscorch.spec
SRPM Name or Url: http://manta.univ.gda.pl/~mgarski/FE/xscorch-0.2.0-3.src.rpm
Description: xscorch is a clone of the classic DOS game, "Scorched Earth". The basic goal is to annihilate enemy tanks using overpowered guns :). Basically, you buy weapons, you target the enemy by adjusting the angle of your turret and firing
power, and you hope to destroy their tank before they destroy yours.
Comment 1 Hans de Goede 2005-08-24 04:37:46 EDT
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?
Comment 2 Hans de Goede 2005-08-24 04:52:16 EDT
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.
Comment 3 Hans de Goede 2005-08-24 10:59:20 EDT
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.
Comment 4 Hans de Goede 2005-08-24 11:01:48 EDT
Created attachment 118068 [details]
modfied spec file for building against gtk2, and which includes a configure patch for proper readline detection.
Comment 5 Hans de Goede 2005-08-24 11:03:55 EDT
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.
Comment 6 Hans de Goede 2005-08-24 11:06:43 EDT
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.
Comment 7 Ralf Corsepius 2005-08-24 12:07:44 EDT
(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.
Comment 8 Marcin Garski 2005-08-24 12:20:54 EDT
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.
Comment 9 Marcin Garski 2005-08-24 12:25:07 EDT
In reply to comment #1.
Hans yes I have access to CVS (since yesterday) :)
I'll look at your package.
Comment 10 Jacob Luna Lundberg 2005-08-24 13:05:57 EDT
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 11 Hans de Goede 2005-08-24 14:46:48 EDT
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.
Comment 12 Jacob Luna Lundberg 2005-08-24 16:20:02 EDT
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.
Comment 13 Hans de Goede 2005-08-25 11:16:17 EDT
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.
Comment 14 Hans de Goede 2005-08-25 11:17:43 EDT
p.s.

I made the chanes to those 2 buffer declerations in _sc_scoring_read_item, the
function which causes the stack overflow.
Comment 15 Michael Schwendt 2005-08-25 12:14:41 EDT
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

Comment 16 Hans de Goede 2005-08-25 13:39:48 EDT
I also tried with +1 to see if there were any off by one errors but that doesn't
do the trick
Comment 17 Jacob Luna Lundberg 2005-08-25 13:46:27 EDT
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.
Comment 18 Hans de Goede 2005-08-25 13:53:44 EDT
long is fine to store a pointer in its just ugly and should be forbidden imho,
thats all.
Comment 19 Michael Schwendt 2005-08-25 14:16:37 EDT
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.
Comment 20 Jacob Luna Lundberg 2005-08-25 14:21:47 EDT
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.
Comment 21 Jacob Luna Lundberg 2005-08-25 14:33:04 EDT
Sorry, I was wrong.  It's strcopyn and they're not in sutil anymore, they're in
libj/jstr .
Comment 22 Michael Schwendt 2005-08-25 17:43:31 EDT
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.
Comment 23 Jacob Luna Lundberg 2005-08-25 18:49:18 EDT
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.
Comment 24 Ralf Corsepius 2005-08-26 00:49:43 EDT
(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
Comment 25 Hans de Goede 2005-08-26 03:33:56 EDT
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.
Comment 26 Hans de Goede 2005-08-26 03:37:55 EDT
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
Comment 27 Hans de Goede 2005-08-26 05:26:15 EDT
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.
Comment 28 Marcin Garski 2005-08-26 07:25:56 EDT
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).
Comment 29 Hans de Goede 2005-08-26 09:03:24 EDT
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@hhs.nl), for some unknown reason
I can't reasign it to myself, even though I've added myself to fedorabugs in the
account system.
Comment 30 Jacob Luna Lundberg 2005-08-26 12:44:37 EDT
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.
Comment 31 Hans de Goede 2005-08-27 03:14:57 EDT
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.
Comment 32 Jacob Luna Lundberg 2005-08-29 13:55:31 EDT
Yup.  That's what I get for being in a hurry to go camping...  I've replaced the
xscorch.org patch.  Thanks.

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