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

[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?

[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?

[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

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

[João Valverde via Wireshark-dev]

On 12/07/21 19:48, Evan Huus wrote:
On Mon, Jul 12, 2021 at 14:42 João Valverde via Wireshark-dev 
mailto:wireshark-dev@wireshark.org>> wrote:




On 12/07/21 19:13, Evan Huus wrote:
 > On Mon, Jul 12, 2021 at 2:05 PM João Valverde via Wireshark-dev
 > mailto:wireshark-dev@wireshark.org>> wrote:
 >>
 >>
 >>
 >> On 12/07/21 16: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?
 >>
 >> I think wmem_packet_scope() should return pinfo->pool.
 >
 > It would have to be converted to a macro (or do a mass-replace anyway
 > to take pinfo as an argument), so I figure using `pinfo->pool`
 > directly in most cases ends up being simplest.

I really don't see it being simplest.


Why?

The motivation for this change is primarily to get rid of the global 
methods so that scope management becomes easier, and so that we could 
e.g. have two packet scopes active at once in a future where we do 
parallel dissection.


OK, I withdraw my objection then. I agree it gets us closer to that goal.

Having wmem_packet_scope return pinfo->pool doesn’t really accomplish 
either of those goals.


Please reconsider.

Either wmem_packet_scope() is created earlier and pinfo->pool =
wmem_packet_scope() or wmem_enter_packet_scope() is passed pinfo->pool
and packet_scope = pinfo->pool.

Either way works fine AFAICT.

 >> Other than that, I don't see a compelling reason to remove the
global
 >> wmem scope methods.
 >>
 >>> Thanks,
 >>> Evan
 >>>
___
 >>> Sent via:    Wireshark-dev mailing list
mailto:wireshark-dev@wireshark.org>>
 >>> 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
mailto:wireshark-dev@wireshark.org>>
 >> 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
mailto:wireshark-dev@wireshark.org>>
 > 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 mailto:wireshark-dev@wireshark.org>>
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: 

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

[Evan Huus]

On Mon, Jul 12, 2021 at 14:42 João Valverde via Wireshark-dev <
wireshark-dev@wireshark.org> wrote:

>
>
> On 12/07/21 19:13, Evan Huus wrote:
> > On Mon, Jul 12, 2021 at 2:05 PM João Valverde via Wireshark-dev
> >  wrote:
> >>
> >>
> >>
> >> On 12/07/21 16: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?
> >>
> >> I think wmem_packet_scope() should return pinfo->pool.
> >
> > It would have to be converted to a macro (or do a mass-replace anyway
> > to take pinfo as an argument), so I figure using `pinfo->pool`
> > directly in most cases ends up being simplest.
>
> I really don't see it being simplest.


Why?

The motivation for this change is primarily to get rid of the global
methods so that scope management becomes easier, and so that we could e.g.
have two packet scopes active at once in a future where we do parallel
dissection.

Having wmem_packet_scope return pinfo->pool doesn’t really accomplish
either of those goals.

Please reconsider.
>
> Either wmem_packet_scope() is created earlier and pinfo->pool =
> wmem_packet_scope() or wmem_enter_packet_scope() is passed pinfo->pool
> and packet_scope = pinfo->pool.
>
> Either way works fine AFAICT.
>
> >> Other than that, I don't see a compelling reason to remove the global
> >> wmem scope methods.
> >>
> >>> 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
> >
>
> ___
> 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?

[João Valverde via Wireshark-dev]

On 12/07/21 19:13, Evan Huus wrote:

On Mon, Jul 12, 2021 at 2:05 PM João Valverde via Wireshark-dev
 wrote:




On 12/07/21 16: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?


I think wmem_packet_scope() should return pinfo->pool.


It would have to be converted to a macro (or do a mass-replace anyway
to take pinfo as an argument), so I figure using `pinfo->pool`
directly in most cases ends up being simplest.


I really don't see it being simplest. Please reconsider.

Either wmem_packet_scope() is created earlier and pinfo->pool = 
wmem_packet_scope() or wmem_enter_packet_scope() is passed pinfo->pool 
and packet_scope = pinfo->pool.


Either way works fine AFAICT.


Other than that, I don't see a compelling reason to remove the global
wmem scope methods.


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



___
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?

[Evan Huus]

On Mon, Jul 12, 2021 at 2:05 PM João Valverde via Wireshark-dev
 wrote:
>
>
>
> On 12/07/21 16: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?
>
> I think wmem_packet_scope() should return pinfo->pool.

It would have to be converted to a macro (or do a mass-replace anyway
to take pinfo as an argument), so I figure using `pinfo->pool`
directly in most cases ends up being simplest.

> Other than that, I don't see a compelling reason to remove the global
> wmem scope methods.
>
> > 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?

[João Valverde via Wireshark-dev]

On 12/07/21 16: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?


I think wmem_packet_scope() should return pinfo->pool.

Other than that, I don't see a compelling reason to remove the global 
wmem scope methods.



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?

[Dr. Matthias St. Pierre]

Sorry Pascal, I overlooked that you already mentioned this difference at the 
end of your reply:

> I had the same idea in the past, mostly because of subtle bugs where 
> Wireshark was using already freed packet memory because of the
> difference between packet and pool scopes (that is documented in README.wmem 
> but still error prone). ...


> -Original Message-
> From: Wireshark-dev  On Behalf Of Dr. 
> Matthias St. Pierre
> Sent: Monday, July 12, 2021 6:38 PM
> To: Developer support list for Wireshark 
> Subject: Re: [Wireshark-dev] Replacing wmem_packet_scope() with pinfo->pool?
> 
> Hi Evan and Pascal,
> 
> > > 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.
> >
> > That's also my understanding.
> 
> FWIW: Incidentally, I asked myself the same question only recently, because I 
> noticed that packet-isakmp dissector used
> wmem_packet_scope() in most locations, except for the following two 
> locations, where it allocates memory for the decrypted
> packets:
> 
> https://gitlab.com/wireshark/wireshark/-/blob/86e2fda11e199b8d0e874147e60a1ba1f0ddb803/epan/dissectors/packet-
> isakmp.c#L2355
> https://gitlab.com/wireshark/wireshark/-/blob/86e2fda11e199b8d0e874147e60a1ba1f0ddb803/epan/dissectors/packet-
> isakmp.c#L5960
> 
> Finally, I found the following explanation in README.wmem, which states that 
> the scope of the pinfo pool is slightly larger than the
> packet scope:
> 
> > 2.3 The Pinfo Pool
> 
> > Certain allocations (such as AT_STRINGZ address allocations and anything 
> > that
> > might end up being passed to add_new_data_source) need their memory to stick
> > around a little longer than the usual packet scope - basically until the
> > next packet is dissected. This is, in fact, the scope of Wireshark's pinfo
> > structure, so the pinfo struct has a 'pool' member which is a wmem pool 
> > scoped
> > to the lifetime of the pinfo struct.
> 
> https://gitlab.com/wireshark/wireshark/-/blob/86e2fda11e199b8d0e874147e60a1ba1f0ddb803/doc/README.wmem#L74-81
> 
> 
> Matthias



smime.p7s
Description: S/MIME cryptographic signature
___
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?

[Dr. Matthias St. Pierre]

Hi Evan and Pascal,

> > 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.
>
> That's also my understanding.

FWIW: Incidentally, I asked myself the same question only recently, because I 
noticed that packet-isakmp dissector used  wmem_packet_scope() in most 
locations, except for the following two locations, where it allocates memory 
for the decrypted packets:

https://gitlab.com/wireshark/wireshark/-/blob/86e2fda11e199b8d0e874147e60a1ba1f0ddb803/epan/dissectors/packet-isakmp.c#L2355
https://gitlab.com/wireshark/wireshark/-/blob/86e2fda11e199b8d0e874147e60a1ba1f0ddb803/epan/dissectors/packet-isakmp.c#L5960

Finally, I found the following explanation in README.wmem, which states that 
the scope of the pinfo pool is slightly larger than the packet scope:

> 2.3 The Pinfo Pool

> Certain allocations (such as AT_STRINGZ address allocations and anything that
> might end up being passed to add_new_data_source) need their memory to stick
> around a little longer than the usual packet scope - basically until the
> next packet is dissected. This is, in fact, the scope of Wireshark's pinfo
> structure, so the pinfo struct has a 'pool' member which is a wmem pool scoped
> to the lifetime of the pinfo struct.

https://gitlab.com/wireshark/wireshark/-/blob/86e2fda11e199b8d0e874147e60a1ba1f0ddb803/doc/README.wmem#L74-81


Matthias



smime.p7s
Description: S/MIME cryptographic signature
___
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?

[Pascal Quantin]

Hi Evan,

Le lun. 12 juil. 2021 à 17:52, Evan Huus  a écrit :

> 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.
>

That's also my understanding.


>
> 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?
>

I had the same idea in the past, mostly because of subtle bugs where
Wireshark was using already freed packet memory because of the difference
between packet and pool scopes (that is documented in README.wmem but still
error prone). Keeping only the later would definitely simplify things (at
the cost of another massive rename like what we did when moving from
ephemeral/seasonal memory to wmem scopes).

Best regards,
Pascal.

PS: nice seeing you active again
___
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

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

[Evan Huus]

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