Re: [Wireshark-dev] Replacing wmem_packet_scope() with pinfo->pool?

2021-07-21 Thread Pascal Quantin
Hi Moshe,

Le mer. 21 juil. 2021 à 17:56, Moshe Kaplan  a
écrit :

> Coverity is complaining that some of the allocations made with pinfo ->
> pool are leaking. Is it possible that the pinfo->pool based allocations are
> not always cleaned up?
>
> As an example, CoverityID 1487512 complains about packet-tcp.c's calls to
> port_with_resolution_to_str leaking:
> https://gitlab.com/wireshark/wireshark/-/blob/master/epan/dissectors/packet-tcp.c#L6500
> .
>

Most likely Coverity's analysis capabilities cannot properly handle a
custom memory allocator like the one we use, where the garbage collector is
performed out of scope of the current functions.
Looking at CID 1487512, this really seems to be the case: Coverity cannot
know that the dynamically allocated buffers that are not stored in any
local/global variable can be freed later on in epan_dissect_cleanup(). So
at first glance it seems like a false positive (like many other ones as far
as I know, maybe there is a way to provide directives to avoid those false
reports). The problem is that using a NULL scope will fallback to
g_malloc'ed memory that will leak, and this might be too subtle for a
simple directive.

Best regards,
Pascal.


> Moshe
>
>
>
> On Wed, Jul 21, 2021 at 11:31 AM Evan Huus  wrote:
>
>> FYI this migration has now begun. Going forward, please use pinfo->pool
>> instead of wmem_packet_scope() in new code when possible. And if anybody
>> has some time, there are lots of existing dissectors left to convert. I
>> expect most of them to be pretty straightforward, just adding pinfo to a
>> few more method signatures as needed.
>>
>> Thanks,
>> Evan
>>
>> On Mon, Jul 12, 2021 at 11:52 Evan Huus  wrote:
>>
>>> I've been thinking recently about starting the process of getting rid
>>> of the "global" wmem scope methods (wmem_packet_scope,
>>> wmem_file_scope, etc) in favour of passing them around in arguments
>>> (or in pinfo, or something). This would let us drop a bunch of
>>> in-scope/out-of-scope tracking and assertion, as well as make the code
>>> more amenable to future refactors like (potentially) concurrency.
>>>
>>> At a first glance, we already have pinfo->pool which maintains the
>>> lifetime of the packet_info object. As far as I can reason, this is
>>> almost/effectively the same as the existing wmem_packet_scope - it
>>> gets cleaned up later in the dissection flow, but there's still only
>>> ever one which gets reused for each packet.
>>>
>>> Is this correct? If so, does it make sense to start replacing
>>> `wmem_packet_scope()` calls with `pinfo->pool` when pinfo is already
>>> in scope?
>>>
>>> Thanks,
>>> Evan
>>>
>>
>> ___
>> Sent via:Wireshark-dev mailing list 
>> Archives:https://www.wireshark.org/lists/wireshark-dev
>> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>>  mailto:wireshark-dev-requ...@wireshark.org
>> ?subject=unsubscribe
>>
> ___
> Sent via:Wireshark-dev mailing list 
> Archives:https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>  mailto:wireshark-dev-requ...@wireshark.org
> ?subject=unsubscribe
>
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe


Re: [Wireshark-dev] Replacing wmem_packet_scope() with pinfo->pool?

2021-07-21 Thread Moshe Kaplan
Coverity is complaining that some of the allocations made with pinfo ->
pool are leaking. Is it possible that the pinfo->pool based allocations are
not always cleaned up?

As an example, CoverityID 1487512 complains about packet-tcp.c's calls to
port_with_resolution_to_str leaking:
https://gitlab.com/wireshark/wireshark/-/blob/master/epan/dissectors/packet-tcp.c#L6500
.

Moshe



On Wed, Jul 21, 2021 at 11:31 AM Evan Huus  wrote:

> FYI this migration has now begun. Going forward, please use pinfo->pool
> instead of wmem_packet_scope() in new code when possible. And if anybody
> has some time, there are lots of existing dissectors left to convert. I
> expect most of them to be pretty straightforward, just adding pinfo to a
> few more method signatures as needed.
>
> Thanks,
> Evan
>
> On Mon, Jul 12, 2021 at 11:52 Evan Huus  wrote:
>
>> I've been thinking recently about starting the process of getting rid
>> of the "global" wmem scope methods (wmem_packet_scope,
>> wmem_file_scope, etc) in favour of passing them around in arguments
>> (or in pinfo, or something). This would let us drop a bunch of
>> in-scope/out-of-scope tracking and assertion, as well as make the code
>> more amenable to future refactors like (potentially) concurrency.
>>
>> At a first glance, we already have pinfo->pool which maintains the
>> lifetime of the packet_info object. As far as I can reason, this is
>> almost/effectively the same as the existing wmem_packet_scope - it
>> gets cleaned up later in the dissection flow, but there's still only
>> ever one which gets reused for each packet.
>>
>> Is this correct? If so, does it make sense to start replacing
>> `wmem_packet_scope()` calls with `pinfo->pool` when pinfo is already
>> in scope?
>>
>> Thanks,
>> Evan
>>
> ___
> Sent via:Wireshark-dev mailing list 
> Archives:https://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
>  mailto:wireshark-dev-requ...@wireshark.org
> ?subject=unsubscribe
>
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe


Re: [Wireshark-dev] Replacing wmem_packet_scope() with pinfo->pool?

2021-07-21 Thread Evan Huus
FYI this migration has now begun. Going forward, please use pinfo->pool
instead of wmem_packet_scope() in new code when possible. And if anybody
has some time, there are lots of existing dissectors left to convert. I
expect most of them to be pretty straightforward, just adding pinfo to a
few more method signatures as needed.

Thanks,
Evan

On Mon, Jul 12, 2021 at 11:52 Evan Huus  wrote:

> I've been thinking recently about starting the process of getting rid
> of the "global" wmem scope methods (wmem_packet_scope,
> wmem_file_scope, etc) in favour of passing them around in arguments
> (or in pinfo, or something). This would let us drop a bunch of
> in-scope/out-of-scope tracking and assertion, as well as make the code
> more amenable to future refactors like (potentially) concurrency.
>
> At a first glance, we already have pinfo->pool which maintains the
> lifetime of the packet_info object. As far as I can reason, this is
> almost/effectively the same as the existing wmem_packet_scope - it
> gets cleaned up later in the dissection flow, but there's still only
> ever one which gets reused for each packet.
>
> Is this correct? If so, does it make sense to start replacing
> `wmem_packet_scope()` calls with `pinfo->pool` when pinfo is already
> in scope?
>
> Thanks,
> Evan
>
___
Sent via:Wireshark-dev mailing list 
Archives:https://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://www.wireshark.org/mailman/options/wireshark-dev
 mailto:wireshark-dev-requ...@wireshark.org?subject=unsubscribe