* Dmitry Shachnev <mity...@gmail.com> [130618 11:11]:
> Thanks for fixing this, I prefer to give someone who knows assembler a
> chance to review this. If we have no such person, I'll sponsor this
> later.
Hello,
I had a look as even if it does not affect Debian ATM, I'm interested
in merging these changes. Thanks a lot for investing this issue and
providing a patch!
Here is a quick review.
> --- a/src/cpu/regs.inc
> +++ b/src/cpu/regs.inc
> +NEWSYM oamread
This exports a pointer so that the next 14 ints can be referred to in:
> - copy_func(buffer, &oamaddr, 14*4);
> + copy_func(buffer, oamread, 14*4);
This reads the following data declared in regs.inc:
- 1 int: oamaddr
- 8 ints: bg[1-4]ptr[xy]
- 8 bytes: Voice[0-7]Disable
- 4 bytes: BG[1-4]16x16t
- 2 ints: SPC700{read, write}
> --- a/src/init.asm
> +++ b/src/init.asm
> +NEWSYM xaread
Same here, this so that the following copy:
> - copy_func(buffer, &xa, 14*4);
> + copy_func(buffer, xaread, 14*4);
can read the next 14 ints: xa, xdb, xpb, xs, xd, xx, xy, flagnz,
flago, flagc, bankkp, Sflagnz, Sflago and Sflagc.
> --- a/src/gblvars.h
> +++ b/src/gblvars.h
> @@ -27,13 +27,14 @@
> -extern unsigned int soundcycleft, spc700read, timer2upd, xa,
> PHnum2writesfxreg;
> -extern unsigned int opcd, HIRQCycNext, oamaddr, curexecstate, nmiprevaddrl;
> +extern unsigned int soundcycleft, timer2upd, xa, PHnum2writesfxreg;
> +extern unsigned char spc700read[], xaread[], opcd[], oamread[];
> +extern unsigned int HIRQCycNext, oamaddr, curexecstate, nmiprevaddrl;
This is equivalent to :
> -extern unsigned int spc700read;
> -extern unsigned int opcd;
> +extern unsigned char spc700read[];
> +extern unsigned char opcd[];
> +extern unsigned char xaread[];
> +extern unsigned char oamread[];
As the symbols spc700read and opcd are now interpreted by address,
their value is equal to the address of the variable, so the following
hunks work:
> - copy_func(buffer, &spc700read, 10*4);
> + copy_func(buffer, spc700read, 10*4);
> copy_func(buffer, &timer2upd, 4);
> copy_func(buffer, &spcnumread, 1);
> - copy_func(buffer, &opcd, 6*4);
> + copy_func(buffer, opcd, 6*4);
However, I think that in the case of char[] variables, you can use &x
for x, so this is unnecessary to remove the & operator (this remark
also applies to oamread and xaread).
> -extern unsigned char sndrot[], SPCRAM[65472], DSPMem[256], SA1Status,
> *SA1RAMArea;
> +extern unsigned char sndrot[], SPCRAM[65472], DSPMem[256], SA1Status,
> *SA1RAMArea, *SPCState;
> --- a/src/initc.c
> +++ b/src/initc.c
> +unsigned char *SPCState = SPCRAM;
> --- a/src/zstate.c
> +++ b/src/zstate.c
> - copy_func(buffer, SPCRAM, PHspcsave);
> + copy_func(buffer, SPCState, PHspcsave);
Is a global necessary? If you put this line in copy_spc_data this
should work, unless you have to put it in a different file to "trick"
the static analysis.
Anyway, something seems off with the size of this variable. Cppcheck
detects an error with it:
http://qa.debian.org/daca/cppcheck/sid/zsnes_1.510+bz2-1.html
My guess is that it's related to the SPC ROM located after SPCRAM.
init65816() accesses 0x40 of those bytes through SPCRAM (I'm not sure
that the 16 ones after have a significance). BTW, I'm surprised that
FORTIFY_SOURCE does not trigger an error on this.
--
Etienne Millon
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1173090
Title:
Buffer overflow in ZSNES since update to raring
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/zsnes/+bug/1173090/+subscriptions
--
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
