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

MixinPreProcessorStandard incorrectly checks STATIC flag on @Unique fields, a crash happens #576

Closed
saharNooby opened this issue Apr 28, 2022 · 1 comment

Comments

@saharNooby
Copy link

Steps to reproduce:

  1. Create a Mixin like this (1.18.2, Mojang names, Fabric):
import net.minecraft.server.packs.repository.Pack;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.Unique;

@Mixin(Pack.class)
public final class TestMixin {

	// Matches name of instance field "id" in the target class
	@Unique
	private static String id;

}
  1. Launch the client

Actual behavior: crash at start

Caused by: org.spongepowered.asm.mixin.transformer.throwables.InvalidMixinException: STATIC modifier of @Shadow field id in ***.TestMixin does not match the target
	at org.spongepowered.asm.mixin.transformer.MixinPreProcessorStandard.attachFields(MixinPreProcessorStandard.java:632)
	at org.spongepowered.asm.mixin.transformer.MixinPreProcessorStandard.attach(MixinPreProcessorStandard.java:302)
	at org.spongepowered.asm.mixin.transformer.MixinPreProcessorStandard.createContextFor(MixinPreProcessorStandard.java:277)
	at org.spongepowered.asm.mixin.transformer.MixinInfo.createContextFor(MixinInfo.java:1289)
	at org.spongepowered.asm.mixin.transformer.MixinApplicatorStandard.apply(MixinApplicatorStandard.java:292)
	at org.spongepowered.asm.mixin.transformer.TargetClassContext.apply(TargetClassContext.java:421)
	at org.spongepowered.asm.mixin.transformer.TargetClassContext.applyMixins(TargetClassContext.java:403)
	at org.spongepowered.asm.mixin.transformer.MixinProcessor.applyMixins(MixinProcessor.java:363)
	... 16 more

Expected behavior: the static field id gets renamed to something unuque and launch proceeds

This is a rare edge case (it occured when I was obfuscating a mod with some custom techiques), but I beleive that this is an incorrect behavior -- what's the point in checking static modifier of a field, if it gets renamed to something unuqie anyway and will not conflict with anything?

I propose that compareFlags should be called only for @Shadow fields, like this:

            if (isShadow && !Bytecode.compareFlags(mixinField, target, Opcodes.ACC_STATIC)) {
                throw new InvalidMixinException(this.mixin, String.format("STATIC modifier of @Shadow field %s in %s does not match the target",
                        mixinField.name, this.mixin));
            }

Or, at least the message could be more clear -- the field is not @Shadow after all. Something like "STATIC modifier of @Unique fields should match modifier of the shadowed field in the target class". After writing it down it sounds really strange :)

Context: line 636 of MixinPreProcessorStandard

@Mumfrey
Copy link
Member

Mumfrey commented Apr 28, 2022

Yeah the entire point of @Unique is "I am adding this and don't intend to accidentall clobber an existing member, so needing it to match doesn't make sense, it's just a bug plain and simple.

modmuss50 pushed a commit to FabricMC/Mixin that referenced this issue Feb 13, 2023
* Fix unique fields crashing when staticness mismatches an existing field

Fixes SpongePowered#576

* Fix mismatching mixin fields silently being dropped

Allowing them to be included will throw a VerifyError
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants