Hi,
Pascal Quantin wrote:
> Le mar. 19 janv. 2021 à 17:45, Joey Salazar via Wireshark-dev <
> wireshark-dev@xxxxxxxxxxxxx> a écrit :
>> In commit 33af2649 [1] we can keep dissecting the contents of the req,
>> adv, and res packets by setting
>> while (plen > 0) { }
>> either in `dissect_git_pdu()` or in `dissect_one_pkt_line()`, but for now
>> in `dissect_git_pdu()` it'd be a bit messy, so wanted to ask for your
>> feedback for getting `dissect_one_pkt_line()` to work properly first.
>>
>> As you can see in pcap 169 [2], it correctly parses the length of the
>> first line as 0x0014 (20 bytes) until `0x0a`, then it's supposed to get the
>> length of the next line by the first 4 hex bytes in that line, but instead
>> of reading the length as 0x0018 (24 bytes) it's reading it as 0x0010 (16
>> bytes), and anyways, this particular line's length actually is 59 bytes.
Interesting. Let me summarize your question: getting the information
in one place here, the relevant code at line 114 looks like
| + while (plen > 0) {
| + proto_tree_add_uint(git_tree, hf_git_packet_len, tvb, offset, 4, plen);
| + offset += 4;
| + plen -= 4;
| +
| + proto_tree_add_item(git_tree, hf_git_packet_data, tvb, offset, plen, ENC_NA);
| + offset += plen;
| + // To-do: add lines for parsing of terminator packet 0000
| + }
The relevant part of the pcap is shown in an image; transcribing
imperfectly, I see
| 0014command=ls-refs\n
| 0018agent=git/2.29.0.rc2
| 0016object-format=sha1
| 0001
[etc]
where \n denotes a newline byte and there are no newlines between
these pkt-lines.
That first pkt-line has 4 bytes for the length, followed by 12 bytes
for "command=ls-refs\n", including newline. So why does this parse as
packet-length: 0x0014
packet data: "command=ls-refs\n"
packet-length: 0x0010
packet data: "agent=[etc]"
?
[...]
> So what is the code leading to this dissection? It does not seem to be
> https://gitlab.com/joeysal/wireshark/-/commit/33af2649927cb5660d4aeb64b9a9e9a58a1823aa
> as dissect_one_pkt_line() seem to read only one line (BTW using a while
> loop in this commit is useless as you are incrementing offset by plen, and
> the code you shared considers that plen includes the 4 bytes of the packet
> length field while your screenshot does not assume that).
This reply is a bit dense, but it contains the hints to move forward:
First, there's the matter of the contract of the dissect_one_pkt_line()
function. The name suggests it would dissect a *single* pkt-line, but
it has this loop in it. What does it actually do? And what do we
want it to do?
That second question is easy to answer: this code will be much easier
to read if dissect_one_pktline dissects a single pkt-line! For
example, if we imagine a contract like
/** Dissects a single pkt-line.
*
* @param[in] tvb Buffer containing a pkt-line.
* @param offset Offset at which to start reading.
* @param[out] tree Tree to attach the dissected pkt-line to.
* @return Number of bytes dissected, or -1 on error.
*/
static int dissect_one_pkt_line(tvbuff_t *tvb, int offset, proto_tree *tree)
then we could call this in a loop, like:
int offset = 0;
while (offset < total length)
offset += dissect_one_pkt_line(tvb, offset, tree);
Obtaining the total length and including some error handling left as
an exercise to the reader.
As for the first question: what does the current code do? The loop at
l114 doesn't modify plen except by subtracting 4 from it. So instead
of reading the pkt-length from the next pkt-line, it assumes it is 4
bytes less. 0x14 - 4 is 0x10, hence the 0x10 as pkt length
assumption.
Thanks and hope that helps,
Jonathan
