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.

Sign up for GitHub

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

Jump to bottom

Common/Network+BBA/Builtin: Cleanup #10831

Merged
merged 9 commits into from
Jul 11, 2022
Merged

Common/Network+BBA/Builtin: Cleanup #10831

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
13957a6
Common/Network: Remove unused constructors
sepalani Jul 10, 2022
b7bd2a4
Common/Network: Use member initializer list
sepalani Jul 10, 2022
4efe1e7
Common/Network: Ensure offsetof is valid
sepalani Jul 10, 2022
6238834
Common/Network: Add InsertObj helper
sepalani Jul 10, 2022
b206f98
Common/Network: Remove unnecessary size checks
sepalani Jul 10, 2022
d902b04
Common/Network: Remove unused offset variables
sepalani Jul 10, 2022
5fdf255
Common/Network: Make Build() methods const
sepalani Jul 10, 2022
b950d03
Common/Network: Update IP checksum since total_len might have changed
sepalani Jul 10, 2022
be2ede6
BBA/BuiltIn: Move functions to anonymous namespace
sepalani Jul 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 14 additions & 2 deletions Source/Core/Common/Network.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,9 +377,15 @@ std::vector<u8> TCPPacket::Build() const
(static_cast<u16>((tcp_options.size() + TCPHeader::SIZE) & 0x3c) << 10);
Common::BitCastPtr<u16>(tcp_ptr + offsetof(TCPHeader, properties)) = htons(tcp_properties);

const u16 ip_total_len = static_cast<u16>(IPv4Header::SIZE + ipv4_options.size() + tcp_length);
const u16 ip_header_size = static_cast<u16>(IPv4Header::SIZE + ipv4_options.size());
const u16 ip_total_len = ip_header_size + tcp_length;
Common::BitCastPtr<u16>(ip_ptr + offsetof(IPv4Header, total_len)) = htons(ip_total_len);

auto ip_checksum_bitcast_ptr =
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super familiar with some of these Dolphin helper classes.

This seems odd to me; I'm probably missing something. I get the first equal is effectively creating the BitCastPtrType and the second equal is memcopy'ing 0 to that pointer. But why bother with that when the 3rd equal you set it to a different value?

Basically, why not do this:

auto ip_checksum_bitcast_ptr = Common::BitCastPtr<u16>(ip_ptr + offsetof(IPv4Header, header_checksum));
ip_checksum_bitcast_ptr = htons(Common::ComputeNetworkChecksum(ip_ptr, ip_header_size));

Copy link
Contributor

Choose a reason for hiding this comment

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

i will awnser for him, the crc calculation include the crc field, it need to be set to 0 for this process

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you but I don't get it. I'd understand if all the bytes were getting zero'd out and then writing just some bytes after. But here the value is being set to 0 and then set to whatever htons returns. Why are both required?

Copy link
Contributor

Choose a reason for hiding this comment

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

ComputeNetworkChecksum() is doing the actual work here and it needs the field set to 0 to return the correct value, but the field when the packet is actually sent needs to be the result of ComputeNetworkChecksum(). Basically what is happening here is:

struct Packet {
 [arbitrary data here]
 u16 checksum
 [more arbitrary data here]
}

Packet p;
p.checksum = 0;
u16 c = CalculateChecksum(p);
p.checksum = c;

Copy link
Contributor

Choose a reason for hiding this comment

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

See https://en.wikipedia.org/wiki/Transmission_Control_Protocol#Checksum_computation for reference. I don't know why the checksum is in the middle of the data either, but that's how it works.

Copy link
Contributor

@iwubcode iwubcode Jul 11, 2022 •
edited

Choose a reason for hiding this comment

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

Ah, I was so focused on the code flow / logic, I didn't stop to think what was going on. Has nothing to do with the memcpy here. We're literally wanting to set the checksum value to 0 to avoid including it in the checksum calculation. I should have seen that. Thanks @AdmiralCurtiss .

This isn't relevant to the PR really because I see this is just a cleanup.

But is there a reason to redo the checksum calculation? Yes, we flipped the bits to send over the wire but flipping the checksum should be adequate? Additionally, just personally I find something like this a little clearer than the pointer arithmetic:

std::vector<u8> TCPPacket::Build() const
{
  TCPPacket packet_result = *this;
  packet_result.tcp_header.properties = htons(packet_result.tcp_header.properties);
  packet_result.ip_header.total_len = htons(packet_result.ip_header.total_len);
  packet_result.ip_header.header_checksum = htons(packet_result.ip_header.header_checksum);
  packet_result.tcp_header.checksum = htons(packet_result.tcp_header.checksum);

  std::vector<u8> result;
  result.reserve(Size());  // Useful not to invalidate .data() pointers
  InsertObj(&result, packet_result);
  return result;
}

I guess it incurs the cost of the copy constructor, but I feel like that's worth the gain in readability.

Additionally, also curious why are we only handling some of the fields? Ex, why isn't source_port, identification, etc being handled?

Copy link
Contributor Author

@sepalani sepalani Jul 11, 2022 •
edited

Choose a reason for hiding this comment

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

Build() is the method used to serialise the packet into bytes.

But is there a reason to redo the checksum calculation? Yes, we flipped the bits to send over the wire but flipping the checksum should be adequate?

Since this method is used for serialisation, before calling it we can change the parts we want of the structure, like updating members such as the TCP options, IP options, payload data and many fields of the IP and TCP headers. That's why we have to make sure the checksums and total_len are up-to-date and haven't changed.

Additionally, just personally I find something like this a little clearer than the pointer arithmetic:

std::vector<u8> TCPPacket::Build() const
{
  TCPPacket packet_result = *this;
  packet_result.tcp_header.properties = htons(packet_result.tcp_header.properties);
  packet_result.ip_header.total_len = htons(packet_result.ip_header.total_len);
  packet_result.ip_header.header_checksum = htons(packet_result.ip_header.header_checksum);
  packet_result.tcp_header.checksum = htons(packet_result.tcp_header.checksum);

Using a copy isn't the only issue, we need several objects to be contiguous in memory (IP header struct, IP options vector, TCP header struct, TCP options vector, data vector), these are needed to compute the checksum easily. That's why working on the vector directly is more straight forward. Otherwise, we have to compute several checksums separately, sum them and set them in the packet_result.

Additionally, also curious why are we only handling some of the fields? Ex, why isn't source_port, identification, etc being handled?

These fields are set in the packet constructor or when necessary by the code in the BBA/BuiltIn file.

Common::BitCastPtr<u16>(ip_ptr + offsetof(IPv4Header, header_checksum));
ip_checksum_bitcast_ptr = u16(0);
ip_checksum_bitcast_ptr = htons(Common::ComputeNetworkChecksum(ip_ptr, ip_header_size));

auto checksum_bitcast_ptr = Common::BitCastPtr<u16>(tcp_ptr + offsetof(TCPHeader, checksum));
checksum_bitcast_ptr = u16(0);
checksum_bitcast_ptr = ComputeTCPNetworkChecksum(
Expand Down Expand Up @@ -421,9 +427,15 @@ std::vector<u8> UDPPacket::Build() const
const u16 udp_length = static_cast<u16>(UDPHeader::SIZE + data.size());
Common::BitCastPtr<u16>(udp_ptr + offsetof(UDPHeader, length)) = htons(udp_length);

const u16 ip_total_len = static_cast<u16>(IPv4Header::SIZE + ipv4_options.size() + udp_length);
const u16 ip_header_size = static_cast<u16>(IPv4Header::SIZE + ipv4_options.size());
const u16 ip_total_len = ip_header_size + udp_length;
Common::BitCastPtr<u16>(ip_ptr + offsetof(IPv4Header, total_len)) = htons(ip_total_len);

auto ip_checksum_bitcast_ptr =
Common::BitCastPtr<u16>(ip_ptr + offsetof(IPv4Header, header_checksum));
ip_checksum_bitcast_ptr = u16(0);
ip_checksum_bitcast_ptr = htons(Common::ComputeNetworkChecksum(ip_ptr, ip_header_size));

auto checksum_bitcast_ptr = Common::BitCastPtr<u16>(udp_ptr + offsetof(UDPHeader, checksum));
checksum_bitcast_ptr = u16(0);
checksum_bitcast_ptr = ComputeTCPNetworkChecksum(
Expand Down