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

Revert "setWaterLevel: add option to change water level outside world… #1882

Closed
wants to merge 2 commits into from

Conversation

Pirulax
Copy link
Contributor

@Pirulax Pirulax commented Nov 29, 2020

According to our research over at #development, and thanks to Dutchman's help the issue seems to be caused by this commit. (Since it modifies the MapInfo packet, and even after reverting bd89570 it still exists I presume this is the commit causing the issue).

As of my knowledge the client bitstream version reflects the one the stream was written with.
Eg.: Server is at 0x10, client at 0x20, the bitstream (that is, bitStream.getVersion() on the client) will at most be 0x10.
If not, then this line causes the issue, since a bit might be true at the point when ReadBit is called, which in turn reads 4 bytes (the float), and hence the whole packet is corrupted.

@Pirulax
Copy link
Contributor Author

Pirulax commented Nov 29, 2020

Uh, I'm not sure what to do about the bitstream version tho.... Older servers might still write that portion(that is, the bit + float), which would cause newer clients(that is, which have this PR) to crash

@TheNormalnij
Copy link
Member

then this line causes

Can you provide better link?

@Pirulax
Copy link
Contributor Author

Pirulax commented Nov 29, 2020

Yes,

pWeaponInfo->SetAccuracy(weaponProperty.data.fAccuracy);

It is the same crash as in #1873

@Pirulax
Copy link
Contributor Author

Pirulax commented Nov 29, 2020

then this line causes

Can you provide better link?

Ah, my bad, I see..
This line:

if (bitStream.Can(eBitStreamVersion::SetWaterLevel_ChangeOutsideWorldLevel))

Just search for it, I can't give a link to it, for whatever reason. It's in CPacketHandler::Packet_MapInfo(NetBitStreamInterface& bitStream)

@TheNormalnij
Copy link
Member

Why bitStream.Can doesn't work there? Server and client use different bitstream version?

@TheNormalnij
Copy link
Member

How to repro related issue?

@Pirulax
Copy link
Contributor Author

Pirulax commented Nov 29, 2020

How to repro related issue?

I have no clue. It seems like connecting to a server running an older version (that is, one that doesnt have this commit) will make the client (which has this commit) crash.

That's the part I dont understand either(from the description):

As of my knowledge the client bitstream version reflects the one the stream was written with.
Eg.: Server is at 0x10, client at 0x20, the bitstream (that is, bitStream.getVersion() on the client) will at most be 0x10.
If not, then this line causes the issue, since a bit might be true at the point when ReadBit is called, which in turn reads 4 bytes (the float), and hence the whole packet is corrupted.

@ds1-e
Copy link
Contributor

ds1-e commented Nov 29, 2020

Yep, connecting is enough to get a crash (i was connecting to server running r20693).

@TheNormalnij
Copy link
Member

But i can connect to older MTA version, So bitStream.Can works good here. Maybe r20693 server is broken itself?

@TheNormalnij
Copy link
Member

r20693 server and client are unavailable to download. Can we confirm this bug for another versions?

@Pirulax
Copy link
Contributor Author

Pirulax commented Nov 29, 2020

They're available for download.
Just go here, and click on:

  • show older files

@TheNormalnij
Copy link
Member

20694 and 20689 only

@Pirulax
Copy link
Contributor Author

Pirulax commented Nov 29, 2020

Interesting.. here, see the Use enum classes for BitStream (#1737).

@TheNormalnij
Copy link
Member

TheNormalnij commented Nov 29, 2020

check next version Make sure BitStream comparisons still work correctly (#1752)

@@ -446,12 +446,12 @@ enum class eBitStreamVersion : unsigned short

// setWaterLevel: add bIncludeWorldSeaLevel and bIncludeOutsideWorldLevel
// 2020-11-03 0x70
SetWaterLevel_ChangeOutsideWorldLevel,
// SetWaterLevel_ChangeOutsideWorldLevel, - Reverted commit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can't do this, because servers with SetWaterLevel_ChangeOutsideWorldLevel bitstream version will send SetWaterLevel_ChangeOutsideWorldLevel packets for clients with PedEnterExit version.

@Pirulax
Copy link
Contributor Author

Pirulax commented Nov 29, 2020

Turns out the issue is only present in r20693, it was later fixed in r20694.

@Pirulax Pirulax closed this Nov 29, 2020
@StrixG StrixG added the invalid This doesn't seem right label Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants