PretendoNetwork / HokakuCTR

On-console NEX RMC traffic dumper for the 3DS
43 stars 5 forks source link

Add title ID to start of packets #6

Closed jonbarrow closed 1 year ago

jonbarrow commented 1 year ago

Adds the games title ID to the beginning of all packets, to identify where they came from. This is useful for tools like NEX Viewer, since to decode the RMC payload we need to know beforehand what NEX version is being used. Since the game is now known based on the title ID, the NEX version can also be easily looked up

DaniElectra commented 1 year ago

LGTM, but can you merge #5 first?

jonbarrow commented 1 year ago

Done

DaniElectra commented 1 year ago

wait no

DaniElectra commented 1 year ago

On RMCLogger.cpp, line 32:

writeBuffer = (u8*)operator new(maxPacketSize + sizeof(PcapPacketHeader));

should probably be

writeBuffer = (u8*)operator new(maxPacketSize + sizeof(PcapPacketHeader) + sizeof(titleID));

right?

jonbarrow commented 1 year ago

I didn't have any issues in my tests without that change but you're probably right, I missed that line entirely tbh

DaniElectra commented 1 year ago

I'm not very familiar with this code, so i will bring @PabloMK7 just to be sure

TraceEntertains commented 1 year ago

Note: i have never worked on this code before, so take everything i mention with a grain of salt :P

First: wouldn't it be easier to maintain in the future if you add the titleid to the packetmetadata as both are technically non-standard and would make the code have less "sizeof()"s?

Second: might it be better to get the title id as a u64 once then run std::to_string(titleId) on it for the folder name?

jonbarrow commented 1 year ago

wouldn't it be easier to maintain in the future if you add the titleid to the packetmetadata as both are technically non-standard and would make the code have less "sizeof()"s?

I did actually do this at first but ended up with the wrong data. Though I may have just screwed something up. I also changed it because it's not a per-packet thing, really, it's a per-game thing so I didn't think it fit the scope of being part of the packet metadata

might it be better to get the title id as a u64 once then run std::to_string(titleId) on it for the folder name?

std::to_string would not produce the same results. Process::GetTitleID(tid) gets the title ID as a hex string, whereas std::to_string would just convert to the title ID to a string of the decimal number (IE, 1125899907041280 instead of 0004000000030800)

PabloMK7 commented 1 year ago

I'll take a look at it with better detail tomorrow. So far I can only tell that it's not possible to edit the PcapHeader because that's the structure Wireshark expects. You can only edit the individual packets data. If you don't want to store the TID in every packet, you can make a single fake packet that stores the TID

jonbarrow commented 1 year ago

You can only edit the individual packets data

That's currently where the title ID is being stored

If you don't want to store the TID in every packet, you can make a single fake packet that stores the TID

I felt like it was fine to store it in every packet, just for simplicity reasons to not have to track if title ID has already been stored or not, plus an extra 8 bytes won't kill anything

TraceEntertains commented 1 year ago

Considering that the titleId is stored in every packet anyways, I don't really see why it couldn't be stored in packetmetadata

Also, I forgot about the std::to_string thing :P

This should properly get the hex string from the int: std::format("{:016X}", tid)

I tested it in an online compiler (it took me way too long to find one with c++20 support)

jonbarrow commented 1 year ago

Considering that the titleId is stored in every packet anyways, I don't really see why it couldn't be stored in packetmetadata

Semantics, mostly. But if Pablo/Dani also disagree with where it's at I'll move it

PabloMK7 commented 1 year ago

Sorry to bring it up again, but maybe it's encoding as 0x60 because the PacketMetadata is uninitialized and grabbing garbage from the stack. Can you revert to the flags struct with the 6 unused bits but add {} just after flags and titleID? (flags{} titleID{}). It's better to have a bitfield in case we want to add more flags in the future.

jonbarrow commented 1 year ago

Sorry to bring it up again, but maybe it's encoding as 0x60 because the PacketMetadata is uninitialized and grabbing garbage from the stack. Can you revert to the flags struct with the 6 unused bits but add {} just after flags and titleID? (flags{} titleID{}). It's better to have a bitfield in case we want to add more flags in the future.

That seems to have done it!

While we're on the topic of future changes, maybe this would be a good time to add some kind of version byte, in case the structure of the data changes again?

TraceEntertains commented 1 year ago

I want to capture some packets for lost world and generations with a person in the discord, but dont want to act too soon on the capturing

Is making a u8 above titleId in packetmetadata defaulting to 1 fine?

(Basically, "u64 titleId, u8 flags" to "u8 version = 1, u64 titleId, u8 flags", or for mk7 with a sent packet, "00 08 03 00 00 00 04 00 00" to "01 00 08 03 00 00 00 04 00 00")

PabloMK7 commented 1 year ago

I want to capture some packets for lost world and generations with a person in the discord, but dont want to act too soon on the capturing

Is making a u8 above titleId in packetmetadata defaulting to 1 fine?

(Basically, "u64 titleId, u8 flags" to "u8 version = 1, u64 titleId, u8 flags", or for mk7 with a sent packet, "00 08 03 00 00 00 04 00 00" to "01 00 08 03 00 00 00 04 00 00")

That sounds reasonable to me, let's see what Jon does for the version

jonbarrow commented 1 year ago

That's exactly what I was gonna do ye