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

0x02 Attribute is possibly unsigned. Made all Signed Ints are now Unsigned. #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

desukuran
Copy link
Contributor

I suspect that 0x02 int32 is unsigned.
Reasons to believe:

  • The client and server will send IP addresses as hex under 0x02, which sometimes can and often exceed the signed limit of 2147483647.
  • Documentation and code from the xfire pidgin plugin treats the 0x02 attribute type as a uint32.
  • Documentation and code from the xfire wireshark plugin (same author) treats the 0x02 attribute type as a uint32.

Reasons to reject:

  • In all of the documentation found, it is never explicitly expressed one way or the other.
  • The disassembly is ambiguous, I see debug prints that say GetInt32() and GetHashValueInt32(), but I cannot confirm nor deny if its looking for uint32 or int32.

pFire:
Preliminary testing has shown for this to connect and operate like normal. More testing is needed to make it gold.

Note:

  • Logging had a -1, it is now 0, I don't know if that impacts anything.

Preliminary testing has shown for this to connect and operate like normal. More testing is needed to make it gold.

* Log -1 is now 0, I don't know if that impacts.
@@ -2,7 +2,7 @@
{
public class FriendRequestModel
{
public int Id { get; set; }
public uint Id { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

Let's keep these as int since it interacts with the database. We may need to add casting in places between our database and the protocol though.

@@ -5,10 +5,10 @@ namespace PFire.Infrastructure.Entities
{
public class Friend : Entity
{
public int MeId { get; set; }
public uint MeId { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

I actually don't think EF supports unsigned ints. Let's try to keep all the interacts with the database to continue to use int.

There's no realistic concern we'll max out the size anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh definitely, I just covered every base in my changes. I will make a new change in a bit.

@@ -0,0 +1,22 @@
using System;
Copy link
Owner

Choose a reason for hiding this comment

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

If you can please also delete the Int32Attribute class in this PR too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will get on it.

@@ -16,7 +16,8 @@ public class XFireAttributeFactory
private XFireAttributeFactory()
{
Add(new StringAttribute());
Add(new Int32Attribute());
//Add(new Int32Attribute());
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
//Add(new Int32Attribute());

Let's just delete this

@darcymiranda
Copy link
Owner

As mentioned in Discord, I'll help test this 👍

Logging had a -1, it is now 0, I don't know if that impacts anything.

No impact -- all good.

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