Fix packet instantiation on 1.20.6

[Brokkonaut]

I am using Unsafe, because the packets were not creatable from a null stream.

Other options would be pre-created serialized data that could be read to instantiate the packets or accessing the constructors via reflection and filling in all the parameters.

Probably not mergeable because of the code style but at least it works.

[derklaro]

I would be more happy with this, if the case of unsafe not being available could be handled more gracefully (this might result in certain packets not being constructable, but prevents failing completely for no reason). Plus, not a fan of having an Unsafe field in the class, can't we hide the direct unsafe field by providing some sort of function that does the thing?

Something like (probably CLASS_INSTANCE_ALLOCATOR assignment will not work like this, needs an extra field outside the try-block - quickly wrote this out of my head):

private static final Function<Class<?>, Object> CLASS_INSTANCE_ALLOCATOR;
static {
  try {
    Class<?> unsafeClass = Class.forName("sun.misc.Unsafe");
    Field theUnsafeField = unsafeClass.getDeclaredField("theUnsafe");
    Object unsafeInstance = Accessors.getFieldAccessor(theUnsafeField).get(null);
    MethodAccessor allocateInstance = Accessors.getMethodAccessor(unsafeClass, "allocateInstance", Class.class);
    CLASS_INSTANCE_ALLOCATOR = clazz -> allocateInstance.invoke(unsafeInstance, clazz);
  } catch (Exception ignored) {
    CLASS_INSTANCE_ALLOCATOR = null;
  }
}

Also, the check if class allocation can be done can then be reduced to CLASS_INSTANCE_ALLOCATOR != null. There are some more edge cases where allocateInstance is not working (for example when trying to allocate an instance of Class), but these should not happen in this case.

Also, as described in JEP 471 the allocateInstance method will remain in medium term, so no big worries when using it here.

One last point: imo the unsafe allocation should be the second-last fallback (before using DefaultInstances), as allocating classes via allocateInstance has some implications (such as fields not being initialized), which seem ok to make construction happen at all, but should be avoided when possible.

[Ingrim4]

I agree with @derklaro. However, could we potentially address this by adding additional "fake" methods to ProtocolLib's Byte Buddy buffer? This approach might allow us to avoid using Unsafe altogether. I haven't investigated this thoroughly yet, so I'm not certain if it’s feasible.

[davidmayr]

I have just checked out your 1.20.6_new branch (that also contains #2940) and tested it out. Whilst protocol lib itself works again (yay) this change does unfortunately break some functionality. Since the constructor of the packets is never called because of your change, the team parameters (in the ClientboundSetPlayerTeamPacket or PacketType.Play.Server.SCOREBOARD_TEAM) are never set, and It's impossible to set team parameters like I'm used to e.g.

InternalStructure struct = packetContainer.getOptionalStructures().read(0).get();
...
packetContainer.getOptionalStructures().write(0, Optional.of(struct));

I'm no protocollib expert, but I don't think you can even do the above anymore without shady reflection at the user side. Although using StructureCache outside the protocollib internals is probably not intended, I still tried it and failed because your change only works if the packet has a stream codec, which the team parameters don't have. Instantiating them by myself is also not possible, since I need a RegistryFriendlyByteBuf. The only thing that remains is implementing the unsafe mess in my code myself. Which sort of destroys the beauty (The not having to do shady reflection stuff part) of protocollib for me.

[davidmayr]

Another thing I found out: I don't know how the JVM works internally with the unsafe allocation, but it seems like you get random values (at least for integers, haven't checked other primitives) which could result in unexpected packets ending up at the client if someone doesn't overwrite some fields

Edit: I was stupid, its all 0

[Ingrim4]

Another thing I found out: I don't know how the JVM works internally with the unsafe allocation, but it seems like you get random values (at least for integers, haven't checked other primitives) which could result in unexpected packets ending up at the client if someone doesn't overwrite some fields

It seems strange to me. I checked the OpenJDK code and, as far as I can tell, the object should be initialized to all zeros. The mem_clear method appears to fill the entire object, excluding the header, with zeros using the fill_to_aligned_words function.

1. Object Unsafe::allocateInstance(Class<?> cls)
2. jobject Unsafe_AllocateInstance(JNIEnv *env, jobject unsafe, jclass cls)
3. instanceOop InstanceKlass::allocate_instance(oop cls, TRAPS)
4. instanceOop InstanceKlass::allocate_instance(TRAPS)
5. oop CollectedHeap::obj_allocate(Klass* klass, size_t size, TRAPS)
6. oop MemAllocator::allocate()
7. oop ObjAllocator::initialize(HeapWord* mem)
8. void MemAllocator::mem_clear(HeapWord* mem)
9. void Copy::fill_to_aligned_words(HeapWord* to, size_t count, juint value = 0)

[davidmayr]

Another thing I found out: I don't know how the JVM works internally with the unsafe allocation, but it seems like you get random values (at least for integers, haven't checked other primitives) which could result in unexpected packets ending up at the client if someone doesn't overwrite some fields

It seems strange to me. I checked the OpenJDK code and, as far as I can tell, the object should be initialized to all zeros. The mem_clear method appears to fill the entire object, excluding the header, with zeros using the fill_to_aligned_words function.

1. [`Object Unsafe::allocateInstance(Class<?> cls)`](https://github.com/openjdk/jdk/blob/0a9d1f8c89e946d99f01549515f6044e53992168/src/java.base/share/classes/jdk/internal/misc/Unsafe.java#L1343)

2. [`jobject Unsafe_AllocateInstance(JNIEnv *env, jobject unsafe, jclass cls)`](https://github.com/openjdk/jdk/blob/0a9d1f8c89e946d99f01549515f6044e53992168/src/hotspot/share/prims/unsafe.cpp#L351)

3. [`instanceOop InstanceKlass::allocate_instance(oop cls, TRAPS)`](https://github.com/openjdk/jdk/blob/0a9d1f8c89e946d99f01549515f6044e53992168/src/hotspot/share/oops/instanceKlass.cpp#L1517)

4. [`instanceOop InstanceKlass::allocate_instance(TRAPS)`](https://github.com/openjdk/jdk/blob/0a9d1f8c89e946d99f01549515f6044e53992168/src/hotspot/share/oops/instanceKlass.cpp#L1511)

5. [`oop CollectedHeap::obj_allocate(Klass* klass, size_t size, TRAPS)`](https://github.com/openjdk/jdk/blob/0a9d1f8c89e946d99f01549515f6044e53992168/src/hotspot/share/gc/shared/collectedHeap.inline.hpp#L34)

6. [`oop MemAllocator::allocate()`](https://github.com/openjdk/jdk/blob/0a9d1f8c89e946d99f01549515f6044e53992168/src/hotspot/share/gc/shared/memAllocator.cpp#L344)

7. [`oop ObjAllocator::initialize(HeapWord* mem)`](https://github.com/openjdk/jdk/blob/0a9d1f8c89e946d99f01549515f6044e53992168/src/hotspot/share/gc/shared/memAllocator.cpp#L379)

8. [`void MemAllocator::mem_clear(HeapWord* mem)`](https://github.com/openjdk/jdk/blob/0a9d1f8c89e946d99f01549515f6044e53992168/src/hotspot/share/gc/shared/memAllocator.cpp#L368)

9. [`void Copy::fill_to_aligned_words(HeapWord* to, size_t count, juint value = 0)`](https://github.com/openjdk/jdk/blob/0a9d1f8c89e946d99f01549515f6044e53992168/src/hotspot/share/utilities/copy.hpp#L271)

You're totally right. My fault for spreading misinformation, thanks for checking.

I tested it right now on a clean project and it actually is 0 now.

The reason I came to my conclusion (that the numbers were randomized) was that when I checked why the packet isn't working correctly anymore I logged the first integer to check what it's set to…

I then forgot that the initial creation of the packet containers (in that specific part of my plugin) is abstracted away and actually modifies the first integer of the created packet containers by default... (it tries to set it to the entity ID. Scoreboard team of course doesn't have one, but most packets sent in that specific part of my code are entity packets, so it checks out to have that. I should probably fix that - but it gets overwritten anyway)

Stuff happens when you don't work on a specific part of your project for over a year 😅

I'm sorry for the confusion I caused, and sorry for the time you wasted researching. It really is all set to 0

[Ingrim4]

Should we use FuzzyReflection here in case some JVM implements this differently?

[Ingrim4]

I discovered the reason why the "fake" ByteBuf method no longer works. Mojang now calls a static method whenever an NbtTag is read from any ByteBuf. Since static methods can't be overridden, we'll need to come up with a new solution. This is particularly problematic because TextComponents are serialized using NbtTags.

Here the static method (static NBTBase FriendlyByteBuf::readNbt(ByteBuf bytebuf, NBTReadLimiter nbtreadlimiter))

    @Nullable
    public static NBTBase readNbt(ByteBuf bytebuf, NBTReadLimiter nbtreadlimiter) {
        try {
            NBTBase nbtbase = NBTCompressedStreamTools.readAnyTag(new ByteBufInputStream(bytebuf), nbtreadlimiter);

            return nbtbase.getId() == 0 ? null : nbtbase;
        } catch (IOException ioexception) {
            throw new EncoderException(ioexception);
        }
    }

One possible solution is to always "return" the bytes for an empty (NbtCompound) TextCompound when the caller is the readNbt method. However, this approach seems quite inelegant.

[Brokkonaut]

I discovered the reason why the "fake" ByteBuf method no longer works. Mojang now calls a static method whenever an NbtTag is read from any ByteBuf. Since static methods can't be overridden, we'll need to come up with a new solution.

Yes that's what i found too - because of this I tried the allocation without a constructor call. But I agree that it is definitely not a good solution.

One possible solution is to always "return" the bytes for an empty (NbtCompound) TextCompound when the caller is the readNbt method. However, this approach seems quite inelegant.

This might be the best option we have.

[dmulloy2]

One possible solution is to always "return" the bytes for an empty (NbtCompound) TextCompound when the caller is the readNbt method. However, this approach seems quite inelegant.

i have no problem with this

[AffectedArc07]

Comment to say fallback to unsafe allocator?