Fwd: Memory Leak: PCAP TX to File

Fwd: Memory Leak: PCAP TX to File

[Stefan Baranoff]

All,

I think I've found a (very minor) memory leak in writing to a PCAP file. In
drivers/net/pcap/rte_eth_pcap.c in open_single_tx_pcap around line 405 a
pcap_t is allocated by pcap_open_dead but is never freed.

I see two obvious fixes:
  1) Free the pcap_t immediately after either on pcap_dump_open success or
failure
  2) Set the pcap_t pointer internals->tx_queue[i]->tx->pcap so when the
eth_dev_stop callback happens the pcap_t is freed.

I believe the first is safe but am not a libpcap guru. Can someone please
confirm this?


Thanks,
Stefan

Re: Fwd: Memory Leak: PCAP TX to File

[Thomas Monjalon]

Cc Ferruh, maintainer of this driver.

26/10/2017 19:44, Stefan Baranoff:
> All,
> 
> I think I've found a (very minor) memory leak in writing to a PCAP file. In
> drivers/net/pcap/rte_eth_pcap.c in open_single_tx_pcap around line 405 a
> pcap_t is allocated by pcap_open_dead but is never freed.
> 
> I see two obvious fixes:
>   1) Free the pcap_t immediately after either on pcap_dump_open success or
> failure
>   2) Set the pcap_t pointer internals->tx_queue[i]->tx->pcap so when the
> eth_dev_stop callback happens the pcap_t is freed.
> 
> I believe the first is safe but am not a libpcap guru. Can someone please
> confirm this?

Do not hesitate to send a patch, even if you are not sure.
You can annotate your patch to discuss your hesitations.
We are a patch-driven community :)

Re: Fwd: Memory Leak: PCAP TX to File

[Ferruh Yigit]

On 10/26/2017 10:44 AM, Stefan Baranoff wrote:
> All,
> 
> I think I've found a (very minor) memory leak in writing to a PCAP file. In
> drivers/net/pcap/rte_eth_pcap.c in open_single_tx_pcap around line 405 a
> pcap_t is allocated by pcap_open_dead but is never freed.

You are right.

> I see two obvious fixes:
>   1) Free the pcap_t immediately after either on pcap_dump_open success or
> failure
>   2) Set the pcap_t pointer internals->tx_queue[i]->tx->pcap so when the
> eth_dev_stop callback happens the pcap_t is freed.

Option 2 looks more proper, since eth_dev_stop() calls pcap_close() which frees
the pcap_t, so nothing special requires to cover the case,

But :),

open_single_tx_pcap() doesn't returns pcap_t back, so this function and
functions using this needs to be updated. I believe doesn't worth to this update
and as far as I can see freeing pcap_t immediately is safe,

So I would suggest lets go with option 1, it is simpler change.

Would you mind providing patch for this fix?

Thanks,
ferruh

> 
> I believe the first is safe but am not a libpcap guru. Can someone please
> confirm this?
> 
> 
> Thanks,
> Stefan
>