captbaritone / webamp

Winamp 2 reimplemented for the browser
https://webamp.org
MIT License
10.02k stars 663 forks source link

Missing opcode 255? #898

Closed captbaritone closed 3 years ago

captbaritone commented 5 years ago

https://archive.org/cors/winampskin_BLAKK/BLAKK.wal has a file boombox2.maki which seems to contain an opcode 255 which we've never seen before. It's possible that some other parsing error before that point is causing a bad reading.

captbaritone commented 5 years ago

I've looked into this, and I think it has to do with calling custom functions. I took the script and reduced it to this:

#include "lib/std.mi"

Function CustomFunction();

CustomFunction() {
    messageBox("Called a custom function", "Success", 0, "");
}

System.onScriptLoaded() {
    // Add a conditional so that the compiler does not inline the function
    if (1) CustomFunction(); 
}
captbaritone commented 5 years ago

The decompiler outputs this, so I'm guessing the decompiler does not know about user functions, and so it's up to us.

/* Note: a decompiler is no invitation to steal code.
   Please respect the the copyright */

#include "std.mi"

System.onScriptLoaded()
{
if(1) {
unknown 255;
pop;
}
return Null;
}
jberg commented 5 years ago

We do have some examples of user functions working.

Maybe it has to do with user functions on user subclasses? Or just subclasses in general?

Also it being 255 made me wonder about whether there is something going on with converting from bytes.

jberg commented 5 years ago

Understanding your example now and seeing it doesn't use subclasses, but it's odd because we have other examples in our tests that use functions

captbaritone commented 5 years ago

Oh, good points. The 255 is definitely suspicious. It seems to be some combination of the conditional and the call to a custom function that causes the issue.

captbaritone commented 5 years ago

@jberg I think you were right to suspect the 255 number. blockStart and blockEnd have opcodes above 255, but we only read a single byte when parsing opcodes.

This is what the decompiler does, so we inherited the bug from that.

jberg commented 5 years ago

Can we read more than 1 byte there? I assume if there was more than 1 byte there everything would be misaligned somehow.

Just FWIW, I have some tests that use what I assume are blocks but they actually don't generate those blockStart/End opcodes, although I guess they couldn't have if it is only a single byte hah, but they were seemingly compiled out.

captbaritone commented 5 years ago

Yeah, I think it probably has to be something more clever than just reading more bytes. I wonder if simple blocks can be compiled away but conditionals inside blocks prevent that compiler optimization. Just a shot in the dark.

jberg commented 5 years ago

So I notice this line that has some special handling for "strange" calls: https://github.com/captbaritone/webamp/blob/f61097aff9f1567f144a3e964477c3f070bfa3a7/modern/src/maki-interpreter/parser.js#L291

Is it possible that the current opcode 255 is only occurring after strange calls and this byte should actually be part of the opcode of the next command in some cases?

captbaritone commented 5 years ago

Interestingly this line in the decompiler checks for an opcode 301, which I think could never happen, given my reading of the code:

jberg commented 5 years ago

Yea, i was wondering if we should even trust that our current issue has anything to do with blocks since the decompiler wouldn't have been able to ever generate those opcodes as is

captbaritone commented 5 years ago

@jberg I had a similar thought. I check and could not find a correlation. There are lots of strange calls and it's not the preceding one in either of the examples I dug up.

jberg commented 5 years ago

@jberg I had a similar thought. I check and could not find a correlation. There are lots of strange calls and it's not the preceding one in either of the examples I dug up.

Bummer. Very weird!

captbaritone commented 5 years ago

I've added a failing test which you can enable to test here.

captbaritone commented 5 years ago

Observation: Not only is the function call missing from the decompiler's output, but the custom function is also missing.

Some things we could try:

  1. Run the decompiler on a bunch of .maiki files from our corpus and see if we ever hit any of the >= 300 opcodes in the code linked above.
  2. Pick one or more of the other .maki files that fail and try to reduce them to a minimal repro. See if they look identical or are somehow different.
  3. Reduce the minimal repro even more and see if we can manually analyze the binary and account for as many bytes as possible both before and after the bad opcode is detected. This might narrow down the bytes that we could be misinterpreting.
  4. Recompiler the minimal repro with each compiler and test if they are all broken, or if it's somehow compiler dependent.

I think I'll start with number 2.

captbaritone commented 4 years ago

Very interesting. I just noticed that the 255 "opcode" (if that really is what it is) comes immediately after we consume a 32bit number for "stack protection". I'm highly suspicious that we are not handling the stack protection correctly.