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

Incorrect null contracts in SidedInvWrapper #1840

Open
Shadows-of-Fire opened this issue Jan 11, 2025 · 4 comments · May be fixed by #1847
Open

Incorrect null contracts in SidedInvWrapper #1840

Shadows-of-Fire opened this issue Jan 11, 2025 · 4 comments · May be fixed by #1847
Labels
1.20.1 Targeted at Minecraft 1.20.1 1.21.1 Targeted at Minecraft 1.21.1 1.21.4 Targeted at Minecraft 1.21.4 bug A bug or error

Comments

@Shadows-of-Fire
Copy link
Contributor

SidedInvWrapper is a capability wrapper for WorldlyContainer, a vanilla interface with the following definition:

public interface WorldlyContainer extends Container {
    int[] getSlotsForFace(Direction side);

    boolean canPlaceItemThroughFace(int index, ItemStack itemStack, @Nullable Direction direction);

    boolean canTakeItemThroughFace(int index, ItemStack stack, Direction direction);
}

As a capability wrapper for the IItemHandler capability, SidedInvWrapper directly accepts a null Direction and passes it through to these methods, which is illegal in two of three cases.

These erroneous calls are in SidedInvWrapper.getSlot and SidedInvWrapper#extractItem. In both cases, SidedInvWrapper needs to fallback to the equivalent behavior from InvWrapper when the direction is null.

This behavior has gone unnoticed for quite a while, simply because none of the vanilla implementation crash when passed null values. This does not change the fact that this is a bug in SidedInvWrapper.

@Shadows-of-Fire Shadows-of-Fire added 1.20.1 Targeted at Minecraft 1.20.1 1.21.1 Targeted at Minecraft 1.21.1 1.21.4 Targeted at Minecraft 1.21.4 bug A bug or error labels Jan 11, 2025
@Technici4n
Copy link
Member

Did it affect you personally or did you just notice by reading the code? 😄

@Shadows-of-Fire
Copy link
Contributor Author

Ran into a crash between Ars and Enchanted where ars was pulling a cap for the null direction, and enchanted was supplying an invwrapper delegating to their WorldlyContainer.

@Technici4n
Copy link
Member

Note that we currently use the regular InvWrapper for a null direction in CapabilityHooks, even when the container is worldly. The only difference for vanilla blocks, is that we probably allow insertion of shulker boxes inside of shulker boxes through the null face.

@TelepathicGrunt
Copy link
Contributor

The two mod's issue report about this for reference:
baileyholl/Ars-Nouveau#1602
Favouriteless/Enchanted#61

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.20.1 Targeted at Minecraft 1.20.1 1.21.1 Targeted at Minecraft 1.21.1 1.21.4 Targeted at Minecraft 1.21.4 bug A bug or error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants