[Pascal Quantin <pascal@xxxxxxxxxxxxx>]

Le ven. 18 oct. 2019 à 20:58, Pascal Quantin <pascal@xxxxxxxxxxxxx> a écrit :

Hi Juliusz,

Le ven. 18 oct. 2019 à 20:51, Juliusz Chroboczek <jch@xxxxxxx> a écrit :

Dear Pascal,

I've just seen your commit dd15b2, which I believe is incorrect.

  https://code.wireshark.org/review/gitweb?p=wireshark.git;a=commit;h=dd15b203c7ec8a8ec5c930cf018c838991ee3182

In dissect_babel_subtlvs, sublen is the length of the TLV body; the full
TLV is of length sublen+2.  At the end of the loop, the code is correct:

        beg += (sublen+2);

Hence, it is perfectly fine to have sublen=0.  Unless I'm wrong, your
commit is incorrect.

Maybe, honestly I do not remember the details as it was done a few months ago. It would require me to dive in once I have the time (so not now).

Could you please either explain why I'm wrong, or else revert commit dd15b2?

I will not revert this commit as it solved/workarounded an infinite loop, as seen in https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=15856

But is you can come up with a better solution that ensures that we do not have the infinite loop while still being correct, you are welcome and I will try to review it

Without even looking at the capture from bug 15856, I can see an infinite loop when subtype = MESSAGE_SUB_PAD1 and sublen = 0:

        if(subtype == MESSAGE_SUB_PAD1){
            beg += sublen;
            continue;
        }

Maybe the test for sublen value should be put only when the subtype is MESSAGE_SUB_PAD1? Or when subtype is different from MESSAGE_SUB_PADN?

As you seem to know this protocol, your feedback is welcome.

Best regards,

Pascal.