Wireshark · Wireshark-dev: Re: [Wireshark-dev] Possible misuse of match_strval_idx

[Evan Huus <eapache@xxxxxxxxx>]

On Sun, Mar 24, 2013 at 6:11 AM, Pascal Quantin
<pascal.quantin@xxxxxxxxx> wrote:
> Le 24/03/2013 00:57, Evan Huus a écrit :
>> On Sat, Mar 23, 2013 at 6:39 PM, Jaap Keuter <jaap.keuter@xxxxxxxxx> wrote:
>>> On 03/23/2013 10:07 PM, Evan Huus wrote:
>>>> Am I correct in thinking that in packet-gsm_a_dtap.c around line 6432,
>>>> if match_strval_idx doesn't find a match then we will access invalid
>>>> memory because idx will be used as an array index with value -1?
>>>>
>>>> Evan
>>> Unless gsm_a_dtap_msg_cc_strings contains 64 entries it seems so yes.
>> Filed as bug #8517.
>
> Hi Evan,
>
> actually those wrong array index will never be used thanks to the
> following lines 6503 and 6622:
> if (msg_str == NULL)
> {
>    ...
> }
> else
> {
>    use ett_tree
> }
> and
> if (msg_str == NULL) return;
> ...
> use dtap_msg_fcn
>
> So the invalid memory accesses will never occur.

I believe the access would still have occurred, since the arrays were
being indexed to store immediately the value in local variables. The
compiler likely optimized that out which made it safe, but a straight
literal compiler would have produced a bug. Either way your commit
fixed it.

> I made it a bit more obvious in r48520.
>
> Unless you spotted another place in the source code, I suggest closing
> bug 8517.

I've added the one other location I'm aware of to 8517, as well as a
few places where the use of match_strval_idx is unnecessary in the
first place (though this is more room for simplification than a bug).

Cheers,
Evan