Comment 16 for bug 1173090

Revision history for this message
Etienne Millon (etienne-millon) wrote : Re: [Bug 1173090] Re: Buffer overflow in ZSNES since update to raring

* Nelson Elhage <email address hidden> [130619 09:26]:
> > 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).
>
> I think that is true (have not tested). I certainly find the version
> without & clearer, but it may be worth having a more minimal patch.
>
> > 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.
>
> Without testing, I suspect that GCC will "see through" the separate
> variable, and the FORTIFY_SOURCE checks will still kick in. Declaring it
> as a char[] and defining it in the .asm file (like I did for the others)
> might be cleaner, though.
>
> > Anyway, something seems off with the size of this variable. Cppcheck
> > detects an error with it:
>
> Yeah, there are those problems and tons of others that GCC finds --
> search https://launchpadlibrarian.net/142716062/buildlog_ubuntu-
> raring-i386.zsnes_1.510%2Bbz2-5ubuntu2fortify2_UPLOADING.txt.gz for
> "will always overflow destination buffer". I only fixed the ones that
> were necessary to make loading a save file work in my testing; Someone
> should probably dig into the rest, but I didn't have the time to
> understand the source code well enough to e.g. change the size in the
> declaration of SPCRAM and be confident that was safe.

I agree with all you said. I created a bug report on the Debian side
to track the inclusion of this patch: http://bugs.debian.org/712790

When this one and http://bugs.debian.org/698990 will be closed, I
believe that there won't be any divergence between Debian and Ubuntu.

--
Etienne Millon