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

Encoder produces different results #75

Open
f3rn0s opened this issue Aug 12, 2024 · 1 comment · May be fixed by #76
Open

Encoder produces different results #75

f3rn0s opened this issue Aug 12, 2024 · 1 comment · May be fixed by #76

Comments

@f3rn0s
Copy link

f3rn0s commented Aug 12, 2024

I'm using this package to build a DHCP packet that gets sent over the wire. I was observing that the packet would be different over different executions. I'm trying to determine why this is the case. chaddr is always the same.

pub fn create_pxe_find_packet(chaddr: [u8; 6]) -> Vec<u8> {
    let message_type = v4::MessageType::Discover;
    let params = v4::DhcpOption::ParameterRequestList(vec![
        v4::OptionCode::SubnetMask,
        v4::OptionCode::Router,
        v4::OptionCode::DomainNameServer,
        v4::OptionCode::TFTPServerName,
        v4::OptionCode::BootfileName,
    ]);

    let mut msg = v4::Message::default();

    msg.set_flags(v4::Flags::default().set_broadcast())
        .set_chaddr(&chaddr);

    msg.opts_mut().insert(v4::DhcpOption::MessageType(v4::MessageType::Discover));

    msg.opts_mut().insert(params);

    msg.opts_mut().insert(v4::DhcpOption::End);

    let mut buf = Vec::new();
    let mut e = Encoder::new(&mut buf);
    msg.encode(&mut e).unwrap();

    buf
}

For example, if I do some testing:

let first_packet = create_pxe_find_packet(chaddr);

while true {
    let pxe_packet = create_pxe_find_packet(chaddr);

    for i in 0..pxe_packet.len() {
        if first_packet[i] != pxe_packet[i] {
            println!("Message mismatch at index {}: {} {}", i, first_packet[i], pxe_packet[i]);
        }
    }
}

I get:

Message mismatch at index 4: 254 187
Message mismatch at index 5: 67 5
Message mismatch at index 6: 97 197
Message mismatch at index 7: 64 51
Message mismatch at index 4: 254 69
Message mismatch at index 5: 67 156
Message mismatch at index 6: 97 35
Message mismatch at index 7: 64 107
Message mismatch at index 240: 55 255
Message mismatch at index 241: 5 53
Message mismatch at index 243: 3 1
Message mismatch at index 244: 6 55
Message mismatch at index 245: 66 5
Message mismatch at index 246: 67 1
Message mismatch at index 247: 53 3
Message mismatch at index 248: 1 6
Message mismatch at index 249: 1 66
Message mismatch at index 250: 255 67
Message mismatch at index 4: 254 157
Message mismatch at index 5: 67 177
Message mismatch at index 6: 97 244
Message mismatch at index 7: 64 82
Message mismatch at index 240: 55 53
Message mismatch at index 241: 5 1
Message mismatch at index 243: 3 255
Message mismatch at index 244: 6 55
Message mismatch at index 245: 66 5
Message mismatch at index 246: 67 1
Message mismatch at index 247: 53 3
Message mismatch at index 248: 1 6
Message mismatch at index 249: 1 66
Message mismatch at index 250: 255 67
Message mismatch at index 4: 254 22
Message mismatch at index 5: 67 152
Message mismatch at index 6: 97 102
Message mismatch at index 7: 64 249
Message mismatch at index 240: 55 255
Message mismatch at index 241: 5 55
Message mismatch at index 242: 1 5
Message mismatch at index 243: 3 1
Message mismatch at index 244: 6 3
Message mismatch at index 245: 66 6
Message mismatch at index 246: 67 66
Message mismatch at index 247: 53 67
Message mismatch at index 248: 1 53
Message mismatch at index 250: 255 1
Message mismatch at index 4: 254 218
Message mismatch at index 5: 67 121
Message mismatch at index 6: 97 1
Message mismatch at index 7: 64 98
Message mismatch at index 240: 55 53
Message mismatch at index 241: 5 1
Message mismatch at index 243: 3 255
Message mismatch at index 244: 6 55
Message mismatch at index 245: 66 5
Message mismatch at index 246: 67 1
Message mismatch at index 247: 53 3
Message mismatch at index 248: 1 6
Message mismatch at index 249: 1 66
Message mismatch at index 250: 255 67
Message mismatch at index 4: 254 90
Message mismatch at index 5: 67 36
Message mismatch at index 6: 97 58
Message mismatch at index 7: 64 236
Message mismatch at index 240: 55 255
Message mismatch at index 241: 5 53
Message mismatch at index 243: 3 1
Message mismatch at index 244: 6 55
Message mismatch at index 245: 66 5
Message mismatch at index 246: 67 1
Message mismatch at index 247: 53 3
Message mismatch at index 248: 1 6
Message mismatch at index 249: 1 66
Message mismatch at index 250: 255 67
Message mismatch at index 4: 254 21
Message mismatch at index 5: 67 234
Message mismatch at index 6: 97 230
Message mismatch at index 7: 64 8
Message mismatch at index 4: 254 56
Message mismatch at index 5: 67 207
Message mismatch at index 6: 97 134
Message mismatch at index 7: 64 255
Message mismatch at index 240: 55 255
Message mismatch at index 241: 5 55
Message mismatch at index 242: 1 5
Message mismatch at index 243: 3 1
Message mismatch at index 244: 6 3

In my case, I can observe this in wireshark because in some requests MessageType isn't being set:

image

@leshow
Copy link
Collaborator

leshow commented Aug 14, 2024

the options are put into a hashmap https://github.com/bluecatengineering/dhcproto/blob/master/src/v4/options.rs#L158 which does not maintain any ordering of elements, so it's likely that when you write out to a buffer the result is not exactly the same byte for byte between encodes.

You don't need to explicitly add the End option, it will be written by the library. It's possible that by manually inserting it, the encode is short-circuiting because as I said, the options are iterated at random from the hashmap. If you could try not inserting the End option, the bytes will still not match but you should see all options written to the buffer. Perhaps we should add a guard so that End can't be inserted

Is the ordering of options important to you? We could switch to a B-Tree map, which maintains order (an order but not the insert order), but it would be a breaking change I think because some HashMap internals are exposed in various iterators.

@f3rn0s f3rn0s linked a pull request Aug 15, 2024 that will close this issue
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 a pull request may close this issue.

2 participants