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.