Skip to content

Conversation

@JuraSciix
Copy link
Contributor

@JuraSciix JuraSciix commented Jun 19, 2025

Problem

Method VarInt::readUnsignedLong() works incorrectly due to an bug in C function readUnsignedVarInt.

Description

The method reads the first 4 bytes (while bitwise shift is less than 32), and ignores the rest.
This is happens because of the compiler makes implicit cast of unsigned char to int in bitwise shift operation. An integer value is shifted by7 * 5 (35) bits and more, so it becomes corrupted.

Reproducing

Code using ext-encoding:

<?php

use pmmp\encoding\ByteBuffer;
use pmmp\encoding\VarInt;

$buffer_1 = chr(1 | 0x80) // 1
    . chr(1 | 0x80)       // 2
    . chr(1 | 0x80)       // 3
    . chr(1 | 0x80)       // 4
    . chr(1);             // 5
$buf_1 = new ByteBuffer($buffer_1);
var_dump(VarInt::readUnsignedLong($buf_1));

$buffer_2 = chr(1 | 0x80) // 1
    . chr(1 | 0x80)       // 2
    . chr(1 | 0x80)       // 3
    . chr(1 | 0x80)       // 4
    . chr(1 | 0x80)       // 5
    . chr(1 | 0x80)       // 6
    . chr(1 | 0x80)       // 7
    . chr(1 | 0x80)       // 8
    . chr(1);             // 9
$buf_2 = new ByteBuffer($buffer_2);
var_dump(VarInt::readUnsignedLong($buf_2));

Output:

int(270549121)
int(287458441)

Code using pocketmine/binaryutils:

<?php

use pocketmine\utils\Binary;

$buffer_1 = chr(1 | 0x80) // 1
    . chr(1 | 0x80)       // 2
    . chr(1 | 0x80)       // 3
    . chr(1 | 0x80)       // 4
    . chr(1);             // 5

$offset_1 = 0;
var_dump(Binary::readUnsignedVarLong($buffer_1, $offset_1));

$buffer_2 = chr(1 | 0x80) // 1
    . chr(1 | 0x80)       // 2
    . chr(1 | 0x80)       // 3
    . chr(1 | 0x80)       // 4
    . chr(1 | 0x80)       // 5
    . chr(1 | 0x80)       // 6
    . chr(1 | 0x80)       // 7
    . chr(1 | 0x80)       // 8
    . chr(1);             // 9

$offset_2 = 0;
var_dump(Binary::readUnsignedVarLong($buffer_2, $offset_2));

Output:

int(270549121)
int(72624976668147841)

And the pocketmine/binaryutils result is right, the ext-encoding result is wrong.

Solution

Add explicit cast of unsigned char to type TValue in evaluations with byte variable in readUnsignedVarInt.

Adds explicit cast of unsigned char to TValue. Analyzer mistakenly implicitly casted unsigned char to 32-bit integer that led to byte loss.
@dktapps
Copy link
Member

dktapps commented Jun 19, 2025

Please add a .phpt test to verify the issue

@JuraSciix
Copy link
Contributor Author

Added read-varint-unsigned-long.phpt

@dktapps
Copy link
Member

dktapps commented Jun 21, 2025

Verified locally, this is good to merge once the build passes 👍🏻

@dktapps dktapps merged commit 106b9f1 into pmmp:master Jun 21, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants