[Wireshark-dev] reassembly, addresses, hash calculation

2014-10-05 Thread Martin Kaiser
Hi,

when I looked at bug 10505, I noticed the following code in
packet-mp2t.c

/* It's possible that a fragment in the same packet set an address already
 * This will change the hash value, we need to make sure it's NULL */

SET_ADDRESS_HF(pinfo-src, AT_NONE, 0, NULL, 0);
SET_ADDRESS_HF(pinfo-dst, AT_NONE, 0, NULL, 0);

[...]
frag_msg = fragment_add_check(mp2t_reassembly_table,
tvb, offset, pinfo, frag_id, NULL,
[...]


Is it really necessary to clear the addresses here in order to make
reassembly work correctly?

Without looking at the reassembly mechanism, I'd say that lower-layer
addresses should be taken into account and mpeg ts packets from
different src/dst addresses shouldn't be reassembled.

mp2t_reassembly_table is initialized with
addresses_reassembly_table_functions. One of these is
fragment_addresses_hash(), which doesn't use the addresses at all.
However, the comparison and key creation functions do use src and dst
addresses...

I'm still tempted to remove the lines where the addresses are cleared.

Thoughts?
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe


Re: [Wireshark-dev] reassembly, addresses, hash calculation

2014-10-05 Thread Evan Huus
Those lines were added earlier this year by Guy Martin (who I've
explicitly copied on this email). The justification in the commit
message at the time was:

If an mp2t packet contains one full subpacket and the fragment of
another one, it happens that the first subpacket will set src or dst to
an ethernet or IP address. Adding the fragment of the second subpacket
will then use this information for calculating the hash in the fragment
table. However, later fragments in other mp2t packets will not have
these info and reassembly will fail.

(For those not familiar with git, I found this information via 'git
blame' which works quite similarly to 'svn blame' and 'cvs annotate').

Evan

On Sun, Oct 5, 2014 at 6:56 AM, Martin Kaiser li...@kaiser.cx wrote:
 Hi,

 when I looked at bug 10505, I noticed the following code in
 packet-mp2t.c

 /* It's possible that a fragment in the same packet set an address already
  * This will change the hash value, we need to make sure it's NULL */

 SET_ADDRESS_HF(pinfo-src, AT_NONE, 0, NULL, 0);
 SET_ADDRESS_HF(pinfo-dst, AT_NONE, 0, NULL, 0);

 [...]
 frag_msg = fragment_add_check(mp2t_reassembly_table,
 tvb, offset, pinfo, frag_id, NULL,
 [...]


 Is it really necessary to clear the addresses here in order to make
 reassembly work correctly?

 Without looking at the reassembly mechanism, I'd say that lower-layer
 addresses should be taken into account and mpeg ts packets from
 different src/dst addresses shouldn't be reassembled.

 mp2t_reassembly_table is initialized with
 addresses_reassembly_table_functions. One of these is
 fragment_addresses_hash(), which doesn't use the addresses at all.
 However, the comparison and key creation functions do use src and dst
 addresses...

 I'm still tempted to remove the lines where the addresses are cleared.

 Thoughts?
 ___
 Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
 Archives:http://www.wireshark.org/lists/wireshark-dev
 Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
  mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe
___
Sent via:Wireshark-dev mailing list wireshark-dev@wireshark.org
Archives:http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe