nsfplay icon indicating copy to clipboard operation
nsfplay copied to clipboard

xgm: Declare NSF version 2 body size as NSF2 PRG size

Open Gumball2415 opened this issue 1 year ago • 5 comments

This prevents NSFe metadata being included as part of the PRG ROM size, which then prevents an access violation error when reading the ROM image in 1MB NSFs.

Gumball2415 avatar Jan 16 '24 05:01 Gumball2415

Would like to address the access violation more directly so that there's fewer ways to crash the program. I'm assuming it's because NES_BANK::SetImage will return false with bankmax > 256, leaving us with a NULL image without checking that error? I'm guessing we want "bankmax = 256" instead of "return false" for that case.

bbbradsmith avatar Jan 16 '24 11:01 bbbradsmith

after looking further into it, it seems to be precisely as you described. i'll try that patch now. in the meantime, i wanted to clarify, is the NSFe footer part of the PRG ROM?

Gumball2415 avatar Jan 16 '24 15:01 Gumball2415

with the bankmax = 256 patch alone, there seems to be heap corruption when delete[]image; is called.

Gumball2415 avatar Jan 16 '24 15:01 Gumball2415

i wanted to clarify, is the NSFe footer part of the PRG ROM?

No, it should not be part of the ROM, though I think it's reasonable to expect that a player (esp. hardware player) might include it instead of zero-padding. (A player like NSFPlay should provide zero-padding.)

with the bankmax = 256 patch alone, there seems to be heap corruption when delete[]image; is called.

That's curious, because I think the problem should be that image was NULL? A delete of NULL shouldn't cause a problem.

Do you have a simple test NSF prepared that you could share that demonstrates the problem?

bbbradsmith avatar Jan 16 '24 18:01 bbbradsmith

nsf.zip this is the test nsf i'm using, which is the bhop demo that i've initially ported to NSF (source).

at the moment it maxes out the 1MB space, and has additional NSF2 NSFe metadata for track title and authors.

That's curious, because I think the problem should be that image was NULL? A delete of NULL shouldn't cause a problem.

it's upon reload when the image contains data.

Gumball2415 avatar Jan 16 '24 19:01 Gumball2415

Thanks, merged. Looking at it, I think the access violation was due to the memcpy to "image", which was using "size" (passed from "bodysize"). I added a more explicit protection of the memcpy, though it should be unnecessary now that bodysize is corrected too.

bbbradsmith avatar Mar 30 '24 16:03 bbbradsmith