Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Illegal characters in path" from GBC rom being treated as an Archive #2587

Closed
TiKevin83 opened this issue Jan 20, 2021 · 8 comments
Closed
Labels
App: EmuHawk Relating to EmuHawk frontend Repro: Affects 2.6 Repro: Fixed/added in 2.6.1 Repro: Regression from 2.5.2 Reproducible bug Should only be added to issues with a `Repro: Affects` label.
Milestone

Comments

@TiKevin83
Copy link
Contributor

TiKevin83 commented Jan 20, 2021

The Emperor's New Groove ROM for GBC is treated as an archive causing an exception in the ROMLoader

Repro

Try to load the above rom

Output

image

@zeromus
Copy link
Contributor

zeromus commented Jan 20, 2021

Extensions other than zip,7z,rar,gz,bz2,xz should not be candidates for testing for IsArchiveness
CommonNonArchiveExtensions in HawkFile.cs is WEAK SAUCE. Remove this and replace with AssumedDefiniteArchiveExtensions and a comment that says "files with this extension must be an archive, or the file can't be loaded".

@YoshiRulz YoshiRulz self-assigned this Jan 20, 2021
@YoshiRulz
Copy link
Member

YoshiRulz commented Jan 20, 2021

Current theory is that these roms happen to have the magic numbers of a TAR archive.


Also reported on Discord with Kirby's Dream Land 2 (also a GB rom):
screencap

@YoshiRulz YoshiRulz added App: EmuHawk Relating to EmuHawk frontend Repro: Affects 2.6 Repro: Regression from 2.5.2 Reproducible bug Should only be added to issues with a `Repro: Affects` label. labels Jan 20, 2021
@zeromus
Copy link
Contributor

zeromus commented Jan 20, 2021

Current theory is you're testing a .gbc extension to see if it's an archive, else you wouldnt get a chance to find a TAR magic number in it

YoshiRulz added a commit that referenced this issue Jan 20, 2021
When `HawkFile` is iterating over archive members of its root trying to bind
one, if it finds one with an invalid name, it will instead bind the root as a
non-archive. This behaviour can be overridden with `forceArchiveMember: true`,
which will result in nothing bound instead of the root being bound.
@YoshiRulz
Copy link
Member

^ thoughts? (also let me know if the debugger attr works, I grabbed it off SO)

@TiKevin83
Copy link
Contributor Author

I'm inclined to agree with zeromus that it's not ideal that you'd even need to try to recover from reading a file with a .gb/,gbc extension as an archive in the first place, but perhaps that commit is still useful for cases like the .lsmv where something not expected to be an archive by extension turns out to be one.

@vadosnaprimer
Copy link
Contributor

If we want to support something, we want to be clear about it, to ourselves and others. lsnes is one of the few emulators that uses plain renamed zip, so it's easier to support it explicitly. Implicit support is accidental and relies on luck, and we want to not depend on luck, for us and users.

@nattthebear
Copy link
Contributor

#2589 gets the HawkFile dependency out of lsmv import, so we can stop worrying about that for this.

@YoshiRulz
Copy link
Member

YoshiRulz commented Jan 21, 2021

Well and truly fixed now. OP's rom, and I assume those from other reports, don't even have the magic bytes of a modern .tar. Instead, SharpCompress was identifying them as the original .tar format (tracked upstream as adamhathcock/sharpcompress#390).


I still think 0ba29e7 is a good idea, just not sure if it's the best implementation. I think the ideal is to try incoming files as roms first, then archives (or concurrently, but the precedence should be for roms). Refactoring RomLoader just doesn't sound appealing to me right now.

@YoshiRulz YoshiRulz pinned this issue Jan 28, 2021
@YoshiRulz YoshiRulz changed the title Exception on GBC ROM being treated as an Archive "Illegal characters in path" from GBC rom being treated as an Archive Jan 28, 2021
@YoshiRulz YoshiRulz modified the milestones: 2.6.2, 2.6.1 Feb 21, 2021
@YoshiRulz YoshiRulz unpinned this issue Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App: EmuHawk Relating to EmuHawk frontend Repro: Affects 2.6 Repro: Fixed/added in 2.6.1 Repro: Regression from 2.5.2 Reproducible bug Should only be added to issues with a `Repro: Affects` label.
Projects
None yet
Development

No branches or pull requests

5 participants