mbox series

[ulogd2,v3,0/2] pcap: prevent crashes when output `FILE *` is null

Message ID 20230316110754.260967-1-jeremy@azazel.net
Headers show
Series pcap: prevent crashes when output `FILE *` is null | expand

Message

Jeremy Sowden March 16, 2023, 11:07 a.m. UTC 
If ulogd2 receives a signal it will attempt to re-open the pcap output
file.  If this fails (because the permissions or ownership have changed
for example), the FILE pointer will be null and when the next packet
comes in, the null pointer will be passed to fwrite and ulogd will
crash.

The first patch simplifies the logic of the code that opens the output
file, and the second avoids closing the existing stream if `fopen`
fails.

Link: https://bugs.launchpad.net/ubuntu/+source/ulogd2/+bug/1429778

Change since v2

 * The first patch is new.
 * In the second patch, just keep the old stream open, rather than
   disabling output and trying to reopen at intervals.

Change since v1

 * Correct subject-prefix.

Jeremy Sowden (2):
  pcap: simplify opening of output file
  pcap: prevent crashes when output `FILE *` is null

 output/pcap/ulogd_output_PCAP.c | 50 +++++++++++++--------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

Comments

Florian Westphal March 16, 2023, 11:36 a.m. UTC | #1 
Jeremy Sowden <jeremy@azazel.net> wrote:
> If ulogd2 receives a signal it will attempt to re-open the pcap output
> file.  If this fails (because the permissions or ownership have changed
> for example), the FILE pointer will be null and when the next packet
> comes in, the null pointer will be passed to fwrite and ulogd will
> crash.
> 
> The first patch simplifies the logic of the code that opens the output
> file, and the second avoids closing the existing stream if `fopen`
> fails.
> 
> Link: https://bugs.launchpad.net/ubuntu/+source/ulogd2/+bug/1429778
> 
> Change since v2
> 
>  * The first patch is new.
>  * In the second patch, just keep the old stream open, rather than
>    disabling output and trying to reopen at intervals.

LGTM, thanks Jeremy.
Florian Westphal March 16, 2023, 11:36 p.m. UTC | #2 
Jeremy Sowden <jeremy@azazel.net> wrote:
> If ulogd2 receives a signal it will attempt to re-open the pcap output
> file.  If this fails (because the permissions or ownership have changed
> for example), the FILE pointer will be null and when the next packet
> comes in, the null pointer will be passed to fwrite and ulogd will
> crash.
> 
> The first patch simplifies the logic of the code that opens the output
> file, and the second avoids closing the existing stream if `fopen`
> fails.
> 
> Link: https://bugs.launchpad.net/ubuntu/+source/ulogd2/+bug/1429778
> 
> Change since v2
> 
>  * The first patch is new.
>  * In the second patch, just keep the old stream open, rather than
>    disabling output and trying to reopen at intervals.

Applied, please double-check the mangling done in patch #1 and send
a followup fix if needed.
Jeremy Sowden March 17, 2023, 11:34 a.m. UTC | #3 
On 2023-03-17, at 00:36:19 +0100, Florian Westphal wrote:
> Jeremy Sowden <jeremy@azazel.net> wrote:
> > If ulogd2 receives a signal it will attempt to re-open the pcap output
> > file.  If this fails (because the permissions or ownership have changed
> > for example), the FILE pointer will be null and when the next packet
> > comes in, the null pointer will be passed to fwrite and ulogd will
> > crash.
> > 
> > The first patch simplifies the logic of the code that opens the output
> > file, and the second avoids closing the existing stream if `fopen`
> > fails.
> > 
> > Link: https://bugs.launchpad.net/ubuntu/+source/ulogd2/+bug/1429778
> > 
> > Change since v2
> > 
> >  * The first patch is new.
> >  * In the second patch, just keep the old stream open, rather than
> >    disabling output and trying to reopen at intervals.
> 
> Applied, please double-check the mangling done in patch #1 and send
> a followup fix if needed.

Thanks, Florian.  LGTM.

J.