2014/1/21 Michal Privoznik <mprivozn@xxxxxxxxxx>:
> On 20.01.2014 15:04, Yuto KAWAMURA wrote:
>> 2014/1/16 Michal Privoznik <mprivozn@xxxxxxxxxx>:
>>> On 15.01.2014 18:06, Yuto KAWAMURA(kawamuray) wrote:
>>>> From: "Yuto KAWAMURA(kawamuray)" <kawamuray.dadada@xxxxxxxxx>
>>>>
>>>> Introduce Wireshark dissector plugin which adds support to Wireshark
>>>> for dissecting libvirt RPC protocol.
>>>> Added following files to build Wireshark dissector from libvirt source
>>>> tree.
>>>> * tools/wireshark/*: Source tree of Wireshark dissector plugin.
>>>>
>>>> Added followings to configure.ac or Makefile.am.
>>>> configure.ac
>>>> * --with-wireshark-dissector: Enable support for building Wireshark
>>>> dissector.
>>>> * --with-ws-plugindir: Specify wireshark plugin directory that dissector
>>>> will installed.
>>>> * Added tools/wireshark/{Makefile,src/Makefile} to AC_CONFIG_FILES.
>>>> Makefile.am
>>>> * Added tools/wireshark/ to SUBDIR.
>>>> ---
>>>> .gitignore | 2 +
>>>> Makefile.am | 3 +-
>>>> cfg.mk | 8 +-
>>>> configure.ac | 72 ++-
>>>> tools/wireshark/Makefile.am | 22 +
>>>> tools/wireshark/README.md | 31 +
>>>> tools/wireshark/src/Makefile.am | 42 ++
>>>> tools/wireshark/src/packet-libvirt.c | 512 ++++++++++++++++
>>>> tools/wireshark/src/packet-libvirt.h | 128 ++++
>>>> tools/wireshark/util/genxdrstub.pl | 1011 +++++++++++++++++++++++++++++++
>>>> tools/wireshark/util/make-dissector-reg | 198 ++++++
>>>> 11 files changed, 2023 insertions(+), 6 deletions(-)
>>>> create mode 100644 tools/wireshark/Makefile.am
>>>> create mode 100644 tools/wireshark/README.md
>>>> create mode 100644 tools/wireshark/src/Makefile.am
>>>> create mode 100644 tools/wireshark/src/packet-libvirt.c
>>>> create mode 100644 tools/wireshark/src/packet-libvirt.h
>>>> create mode 100755 tools/wireshark/util/genxdrstub.pl
>>>> create mode 100755 tools/wireshark/util/make-dissector-reg
>>>>
>>>
>>>> diff --git a/tools/wireshark/src/packet-libvirt.c b/tools/wireshark/src/packet-libvirt.c
>>>> new file mode 100644
>>>> index 0000000..2d0350c
>>>> --- /dev/null
>>>> +++ b/tools/wireshark/src/packet-libvirt.c
>>>
>>>> +static void
>>>> +dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
>>>> +{
>>>> + goffset offset;
>>>> + guint32 prog, proc, type, serial, status;
>>>> + const value_string *vs;
>>>> +
>>>> + col_set_str(pinfo->cinfo, COL_PROTOCOL, "Libvirt");
>>>> + col_clear(pinfo->cinfo, COL_INFO);
>>>> +
>>>> + offset = 4; /* End of length field */
>>>> + prog = tvb_get_ntohl(tvb, offset); offset += 4;
>>>> + offset += 4; /* Ignore version header field */
>>>> + proc = tvb_get_ntohl(tvb, offset); offset += 4;
>>>> + type = tvb_get_ntohl(tvb, offset); offset += 4;
>>>> + serial = tvb_get_ntohl(tvb, offset); offset += 4;
>>>> + status = tvb_get_ntohl(tvb, offset); offset += 4;
>>>> +
>>>> + col_add_fstr(pinfo->cinfo, COL_INFO, "Prog=%s",
>>>> + val_to_str(prog, program_strings, "%x"));
>>>> +
>>>> + vs = get_program_data(prog, VIR_PROGRAM_PROCSTRINGS);
>>>> + if (vs == NULL) {
>>>> + col_append_fstr(pinfo->cinfo, COL_INFO, " Proc=%u", proc);
>>>> + } else {
>>>> + col_append_fstr(pinfo->cinfo, COL_INFO, " Proc=%s", val_to_str(proc, vs, "%d"));
>>>> + }
>>>> +
>>>> + col_append_fstr(pinfo->cinfo, COL_INFO, " Type=%s Serial=%u Status=%s",
>>>> + val_to_str(type, type_strings, "%d"), serial,
>>>> + val_to_str(status, status_strings, "%d"));
>>>> +
>>>> + if (tree) {
>>>> + gint hf_proc;
>>>> + proto_item *ti;
>>>> + proto_tree *libvirt_tree;
>>>> +
>>>> + ti = proto_tree_add_item(tree, proto_libvirt, tvb, 0, tvb_length(tvb), ENC_NA);
>>>> + libvirt_tree = proto_item_add_subtree(ti, ett_libvirt);
>>>> +
>>>> + offset = 0;
>>>> + proto_tree_add_item(libvirt_tree, hf_libvirt_length, tvb, offset, 4, ENC_NA); offset += 4;
>>>> + proto_tree_add_item(libvirt_tree, hf_libvirt_program, tvb, offset, 4, ENC_NA); offset += 4;
>>>> + proto_tree_add_item(libvirt_tree, hf_libvirt_version, tvb, offset, 4, ENC_NA); offset += 4;
>>>> +
>>>> + hf_proc = *(int *)get_program_data(prog, VIR_PROGRAM_PROCHFVAR);
>>>
>>> There's a possible NULL dereference here. get_program might return NULL in case @prog is not one of REMOTE, QEMU, LXC, KEEPALIVE. This can possibly happen and it did for me indeed:
>>>
>>> Program terminated with signal 11, Segmentation fault.
>>> #0 0x00007fac1cc60a86 in dissect_libvirt_message (tvb=0x387acc0, pinfo=0x7fffb9a03100, tree=0x3883090) at packet-libvirt.c:396
>>> 396 hf_proc = *(int *)get_program_data(prog, VIR_PROGRAM_PROCHFVAR);
>>> (gdb) bt
>>> #0 0x00007fac1cc60a86 in dissect_libvirt_message (tvb=0x387acc0, pinfo=0x7fffb9a03100, tree=0x3883090) at packet-libvirt.c:396
>>> #1 0x00000034469198fe in tcp_dissect_pdus () from /usr/lib64/libwireshark.so.3
>>> #2 0x00007fac1cc60c0c in dissect_libvirt (tvb=0x3879aa0, pinfo=0x7fffb9a03100, tree=0x3883090) at packet-libvirt.c:424
>>> #3 0x00000034462dff54 in ?? () from /usr/lib64/libwireshark.so.3
>>> #4 0x00000034462e0608 in ?? () from /usr/lib64/libwireshark.so.3
>>> #5 0x00000034462e0e0c in ?? () from /usr/lib64/libwireshark.so.3
>>> #6 0x00000034462e0e67 in dissector_try_uint () from /usr/lib64/libwireshark.so.3
>>> ...
>>>
>>> (gdb) p /x VIR_PROGRAM_PROCHFVAR
>>> $1 = 0x0
>>> (gdb) p /x prog
>>> $2 = 0xd21c44f9
>>>
>>> This happened if I used sasl for authentication (auth_tcp="sasl" listen_tcp=1 listen_tls=0)
>>>
>>>> + if (hf_proc == -1) {
>>>> + proto_tree_add_none_format(libvirt_tree, -1, tvb, offset, 4, "Unknown proc: %u", proc);
>>>> + } else {
>>>> + proto_tree_add_item(libvirt_tree, hf_proc, tvb, offset, 4, ENC_NA);
>>>> + }
>>>> + offset += 4;
>>>
>>> I've tried to fixed this by applying:
>>> @@ -381,7 +381,7 @@ dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
>>> val_to_str(status, status_strings, "%d"));
>>>
>>> if (tree) {
>>> - gint hf_proc;
>>> + gint *hf_proc;
>>> proto_item *ti;
>>> proto_tree *libvirt_tree;
>>>
>>> @@ -393,11 +393,11 @@ dissect_libvirt_message(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
>>> proto_tree_add_item(libvirt_tree, hf_libvirt_program, tvb, offset, 4, ENC_NA); offset += 4;
>>> proto_tree_add_item(libvirt_tree, hf_libvirt_version, tvb, offset, 4, ENC_NA); offset += 4;
>>>
>>> - hf_proc = *(int *)get_program_data(prog, VIR_PROGRAM_PROCHFVAR);
>>> - if (hf_proc == -1) {
>>> + hf_proc = (int *)get_program_data(prog, VIR_PROGRAM_PROCHFVAR);
>>> + if (!hf_proc || *hf_proc == -1) {
>>> proto_tree_add_none_format(libvirt_tree, -1, tvb, offset, 4, "Unknown proc: %u", proc);
>>> } else {
>>> - proto_tree_add_item(libvirt_tree, hf_proc, tvb, offset, 4, ENC_NA);
>>> + proto_tree_add_item(libvirt_tree, *hf_proc, tvb, offset, 4, ENC_NA);
>>> }
>>> offset += 4;
>>>
>>>
>>> but it worked only partially as then I was seeing this error messages (but no segfault :) ):
>>>
>>> 12:29:09 Warn Dissector bug, protocol libvirt, in packet 32: proto.c:1854: failed assertion "(guint)hfindex < gpa_hfinfo.len" (Unregistered hf!)
>>>
>>> The error message kept repeating over and over again. The rest of the patch looks okay. Once you fix this (sending a follow-up patch is just fine) I'll push this. Thanks for taking care of the dissector!
>>>
>>> Michal
>>>
>>
>> Thanks for review Michal.
>> Fix patch for this bug is following.
>> Regoards.
>>
>> diff --git a/tools/wireshark/src/packet-libvirt.c
>> b/tools/wireshark/src/packet-libvirt.c
>> index 2d0350c..d64ecce 100644
>> --- a/tools/wireshark/src/packet-libvirt.c
>> +++ b/tools/wireshark/src/packet-libvirt.c
>> @@ -39,6 +39,7 @@ static int proto_libvirt = -1;
>> static int hf_libvirt_length = -1;
>> static int hf_libvirt_program = -1;
>> static int hf_libvirt_version = -1;
>> +static int hf_libvirt_procedure = -1;
>> static int hf_libvirt_type = -1;
>> static int hf_libvirt_serial = -1;
>> static int hf_libvirt_status = -1;
>> @@ -381,7 +382,7 @@ dissect_libvirt_message(tvbuff_t *tvb, packet_info
>> *pinfo, proto_tree *tree)
>> val_to_str(status, status_strings, "%d"));
>>
>> if (tree) {
>> - gint hf_proc;
>> + gint *hf_proc;
>> proto_item *ti;
>> proto_tree *libvirt_tree;
>>
>> @@ -393,11 +394,12 @@ dissect_libvirt_message(tvbuff_t *tvb,
>> packet_info *pinfo, proto_tree *tree)
>> proto_tree_add_item(libvirt_tree, hf_libvirt_program, tvb,
>> offset, 4, ENC_NA); offset += 4;
>> proto_tree_add_item(libvirt_tree, hf_libvirt_version, tvb,
>> offset, 4, ENC_NA); offset += 4;
>>
>> - hf_proc = *(int *)get_program_data(prog, VIR_PROGRAM_PROCHFVAR);
>> - if (hf_proc == -1) {
>> - proto_tree_add_none_format(libvirt_tree, -1, tvb, offset,
>> 4, "Unknown proc: %u", proc);
>> + hf_proc = (int *)get_program_data(prog, VIR_PROGRAM_PROCHFVAR);
>> + if (hf_proc != NULL && *hf_proc != -1) {
>> + proto_tree_add_item(libvirt_tree, *hf_proc, tvb, offset,
>> 4, ENC_NA);
>> } else {
>> - proto_tree_add_item(libvirt_tree, hf_proc, tvb, offset, 4, ENC_NA);
>> + /* No string representation, but still useful displaying
>> proc number */
>> + proto_tree_add_item(libvirt_tree, hf_libvirt_procedure,
>> tvb, offset, 4, ENC_NA);
>> }
>> offset += 4;
>>
>> @@ -446,6 +448,12 @@ proto_register_libvirt(void)
>> NULL, 0x0,
>> NULL, HFILL}
>> },
>> + { &hf_libvirt_procedure,
>> + { "procedure", "libvirt.procedure",
>> + FT_INT32, BASE_DEC,
>> + NULL, 0x0,
>> + NULL, HFILL}
>> + },
>> { &hf_libvirt_type,
>> { "type", "libvirt.type",
>> FT_INT32, BASE_DEC,
>>
>>
>
> Yes, this fixes the problem. ACK. I'm squashing this in and pushing the
> series. Thanks!
>
> Michal
Thank you Michal.
kawamuray
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list