| Message ID | 20230316110754.260967-2-jeremy@azazel.net |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | pcap: prevent crashes when output `FILE *` is null | expand |
Jeremy Sowden <jeremy@azazel.net> wrote: > Instead of statting the file, and choosing the mode with which to open > it and whether to write the PCAP header based on the result, always open > it with mode "a" and _then_ stat it. This simplifies the flow-control > and avoids a race between statting and opening. > > Signed-off-by: Jeremy Sowden <jeremy@azazel.net> > --- > output/pcap/ulogd_output_PCAP.c | 42 ++++++++++++--------------------- > 1 file changed, 15 insertions(+), 27 deletions(-) > > diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c > index e7798f20c8fc..220fc6dec5fe 100644 > --- a/output/pcap/ulogd_output_PCAP.c > +++ b/output/pcap/ulogd_output_PCAP.c > @@ -220,33 +220,21 @@ static int append_create_outfile(struct ulogd_pluginstance *upi) > { > + struct stat st_of; > + > + pi->of = fopen(filename, "a"); > + if (!pi->of) { > + ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", > + filename, > + strerror(errno)); > + return -EPERM; > + } > + if (fstat(fileno(pi->of), &st_of) == 0 && st_of.st_size == 0) { > + if (!write_pcap_header(pi)) { > + ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n", > + strerror(errno)); > + return -ENOSPC; LGTM, but should that fclose() before -ENOSPC?
Florian Westphal <fw@strlen.de> wrote: > Jeremy Sowden <jeremy@azazel.net> wrote: > > Instead of statting the file, and choosing the mode with which to open > > it and whether to write the PCAP header based on the result, always open > > it with mode "a" and _then_ stat it. This simplifies the flow-control > > and avoids a race between statting and opening. > > > > Signed-off-by: Jeremy Sowden <jeremy@azazel.net> > > --- > > output/pcap/ulogd_output_PCAP.c | 42 ++++++++++++--------------------- > > 1 file changed, 15 insertions(+), 27 deletions(-) > > > > diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c > > index e7798f20c8fc..220fc6dec5fe 100644 > > --- a/output/pcap/ulogd_output_PCAP.c > > +++ b/output/pcap/ulogd_output_PCAP.c > > @@ -220,33 +220,21 @@ static int append_create_outfile(struct ulogd_pluginstance *upi) > > { > > > + struct stat st_of; > > + > > + pi->of = fopen(filename, "a"); > > + if (!pi->of) { > > + ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", > > + filename, > > + strerror(errno)); > > + return -EPERM; > > + } > > + if (fstat(fileno(pi->of), &st_of) == 0 && st_of.st_size == 0) { > > + if (!write_pcap_header(pi)) { > > + ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n", > > + strerror(errno)); > > + return -ENOSPC; > > LGTM, but should that fclose() before -ENOSPC? AFAICS this doesn't really matter, ulogd will exit(). SIGHUP case doesn't handle errors.
On Thu, Mar 16, 2023 at 11:07:53AM +0000, Jeremy Sowden wrote: > Instead of statting the file, and choosing the mode with which to open > it and whether to write the PCAP header based on the result, always open > it with mode "a" and _then_ stat it. This simplifies the flow-control > and avoids a race between statting and opening. > > Signed-off-by: Jeremy Sowden <jeremy@azazel.net> > --- > output/pcap/ulogd_output_PCAP.c | 42 ++++++++++++--------------------- > 1 file changed, 15 insertions(+), 27 deletions(-) > > diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c > index e7798f20c8fc..220fc6dec5fe 100644 > --- a/output/pcap/ulogd_output_PCAP.c > +++ b/output/pcap/ulogd_output_PCAP.c > @@ -220,33 +220,21 @@ static int append_create_outfile(struct ulogd_pluginstance *upi) > { > struct pcap_instance *pi = (struct pcap_instance *) &upi->private; > char *filename = upi->config_kset->ces[0].u.string; > - struct stat st_dummy; > - int exist = 0; > - > - if (stat(filename, &st_dummy) == 0 && st_dummy.st_size > 0) > - exist = 1; > - > - if (!exist) { > - pi->of = fopen(filename, "w"); > - if (!pi->of) { > - ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", > - filename, > - strerror(errno)); > - return -EPERM; > - } > - if (!write_pcap_header(pi)) { > - ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n", > - strerror(errno)); > - return -ENOSPC; > - } > - } else { > - pi->of = fopen(filename, "a"); > - if (!pi->of) { > - ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", > - filename, > - strerror(errno)); > - return -EPERM; > - } > + struct stat st_of; > + > + pi->of = fopen(filename, "a"); > + if (!pi->of) { > + ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", > + filename, > + strerror(errno)); > + return -EPERM; > + } > + if (fstat(fileno(pi->of), &st_of) == 0 && st_of.st_size == 0) { > + if (!write_pcap_header(pi)) { ^^^^ coding style nitpick, it can be fixed before applying it. > + ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n", > + strerror(errno)); > + return -ENOSPC; > + } > } > > return 0; > -- > 2.39.2 >
On 2023-03-16, at 20:02:15 +0100, Pablo Neira Ayuso wrote: > On Thu, Mar 16, 2023 at 11:07:53AM +0000, Jeremy Sowden wrote: > > Instead of statting the file, and choosing the mode with which to open > > it and whether to write the PCAP header based on the result, always open > > it with mode "a" and _then_ stat it. This simplifies the flow-control > > and avoids a race between statting and opening. > > > > Signed-off-by: Jeremy Sowden <jeremy@azazel.net> > > --- > > output/pcap/ulogd_output_PCAP.c | 42 ++++++++++++--------------------- > > 1 file changed, 15 insertions(+), 27 deletions(-) > > > > diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c > > index e7798f20c8fc..220fc6dec5fe 100644 > > --- a/output/pcap/ulogd_output_PCAP.c > > +++ b/output/pcap/ulogd_output_PCAP.c > > @@ -220,33 +220,21 @@ static int append_create_outfile(struct ulogd_pluginstance *upi) > > { > > struct pcap_instance *pi = (struct pcap_instance *) &upi->private; > > char *filename = upi->config_kset->ces[0].u.string; > > - struct stat st_dummy; > > - int exist = 0; > > - > > - if (stat(filename, &st_dummy) == 0 && st_dummy.st_size > 0) > > - exist = 1; > > - > > - if (!exist) { > > - pi->of = fopen(filename, "w"); > > - if (!pi->of) { > > - ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", > > - filename, > > - strerror(errno)); > > - return -EPERM; > > - } > > - if (!write_pcap_header(pi)) { > > - ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n", > > - strerror(errno)); > > - return -ENOSPC; > > - } > > - } else { > > - pi->of = fopen(filename, "a"); > > - if (!pi->of) { > > - ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", > > - filename, > > - strerror(errno)); > > - return -EPERM; > > - } > > + struct stat st_of; > > + > > + pi->of = fopen(filename, "a"); > > + if (!pi->of) { > > + ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", > > + filename, > > + strerror(errno)); > > + return -EPERM; > > + } > > + if (fstat(fileno(pi->of), &st_of) == 0 && st_of.st_size == 0) { > > + if (!write_pcap_header(pi)) { > ^^^^ > coding style nitpick, Whoops. Not sure what happened there. > it can be fixed before applying it. Please. J. > > + ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n", > > + strerror(errno)); > > + return -ENOSPC; > > + } > > } > > > > return 0; > > -- > > 2.39.2 > > >
diff --git a/output/pcap/ulogd_output_PCAP.c b/output/pcap/ulogd_output_PCAP.c index e7798f20c8fc..220fc6dec5fe 100644 --- a/output/pcap/ulogd_output_PCAP.c +++ b/output/pcap/ulogd_output_PCAP.c @@ -220,33 +220,21 @@ static int append_create_outfile(struct ulogd_pluginstance *upi) { struct pcap_instance *pi = (struct pcap_instance *) &upi->private; char *filename = upi->config_kset->ces[0].u.string; - struct stat st_dummy; - int exist = 0; - - if (stat(filename, &st_dummy) == 0 && st_dummy.st_size > 0) - exist = 1; - - if (!exist) { - pi->of = fopen(filename, "w"); - if (!pi->of) { - ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", - filename, - strerror(errno)); - return -EPERM; - } - if (!write_pcap_header(pi)) { - ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n", - strerror(errno)); - return -ENOSPC; - } - } else { - pi->of = fopen(filename, "a"); - if (!pi->of) { - ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", - filename, - strerror(errno)); - return -EPERM; - } + struct stat st_of; + + pi->of = fopen(filename, "a"); + if (!pi->of) { + ulogd_log(ULOGD_ERROR, "can't open pcap file %s: %s\n", + filename, + strerror(errno)); + return -EPERM; + } + if (fstat(fileno(pi->of), &st_of) == 0 && st_of.st_size == 0) { + if (!write_pcap_header(pi)) { + ulogd_log(ULOGD_ERROR, "can't write pcap header: %s\n", + strerror(errno)); + return -ENOSPC; + } } return 0;
Instead of statting the file, and choosing the mode with which to open it and whether to write the PCAP header based on the result, always open it with mode "a" and _then_ stat it. This simplifies the flow-control and avoids a race between statting and opening. Signed-off-by: Jeremy Sowden <jeremy@azazel.net> --- output/pcap/ulogd_output_PCAP.c | 42 ++++++++++++--------------------- 1 file changed, 15 insertions(+), 27 deletions(-)