Skip to content

Fix bitwise operation in readUnsignedVarInt#17

Merged
dktapps merged 7 commits intopmmp:masterfrom
JuraSciix:patch-1
Jun 21, 2025
Merged

Fix bitwise operation in readUnsignedVarInt#17
dktapps merged 7 commits intopmmp:masterfrom
JuraSciix:patch-1

Conversation

@JuraSciix
Copy link
Contributor

@JuraSciix JuraSciix commented Jun 19, 2025 •
edited
Loading

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.

@JuraSciix
Fixes bitwise operation in readUnsignedVarInt
090f6a3 
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

JuraSciix commented Jun 19, 2025

Added read-varint-unsigned-long.phpt

dktapps
dktapps reviewed Jun 20, 2025
View reviewed changes
@dktapps
Copy link
Member

dktapps commented Jun 21, 2025

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

dktapps
dktapps reviewed Jun 21, 2025
View reviewed changes
@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

Reviewers

@dktapps dktapps dktapps left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@JuraSciix @dktapps