search

bbbradsmith / nsfplay

Nintendo NES sound file NSF music player
https://bbbradsmith.github.io/nsfplay/
277 stars 42 forks source link

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

Closed Gumball2415 closed 4 months ago

Gumball2415 commented 7 months ago

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.

bbbradsmith commented 7 months ago

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.

Gumball2415 commented 7 months ago

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 commented 7 months ago

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

bbbradsmith commented 7 months ago

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?

Gumball2415 commented 7 months ago

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.

bbbradsmith commented 4 months ago

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.