[0/8] Coverity fixes for vchan-socket-proxy

Message

Jason Andryuk May 25, 2020, 2:49 a.m. UTC

This series addresses some Coverity reports.  To handle closing FDs, a
state struct is introduced to track FDs closed in both main() and
data_loop().

Jason Andryuk (8):
  vchan-socket-proxy: Ensure UNIX path NUL terminated
  vchan-socket-proxy: Check xs_watch return value
  vchan-socket-proxy: Unify main return value
  vchan-socket-proxy: Use a struct to store state
  vchan-socket-proxy: Switch data_loop() to take state
  vchan-socket-proxy: Set closed FDs to -1
  vchan-socket-proxy: Cleanup resources on exit
  vchan-socket-proxy: Handle closing shared input/output_fd

 tools/libvchan/vchan-socket-proxy.c | 164 ++++++++++++++++++----------
 1 file changed, 106 insertions(+), 58 deletions(-)

Comments

Jason Andryuk May 25, 2020, 10:36 p.m. UTC | #1

On Sun, May 24, 2020 at 10:50 PM Jason Andryuk <[e-mail]> wrote:
>
> This series addresses some Coverity reports.  To handle closing FDs, a
> state struct is introduced to track FDs closed in both main() and
> data_loop().

I've realized the changes here are insufficient to handle the FD
leaks.  That is, the accept()-ed FDs need to be closed inside the for
loop so they aren't leaked with each iteration.  I'll re-work for a
v2.

-Jason

Jason Andryuk May 28, 2020, 2:59 a.m. UTC | #2

On Mon, May 25, 2020 at 6:36 PM Jason Andryuk <[e-mail]> wrote:
>
> On Sun, May 24, 2020 at 10:50 PM Jason Andryuk <[e-mail]> wrote:
> >
> > This series addresses some Coverity reports.  To handle closing FDs, a
> > state struct is introduced to track FDs closed in both main() and
> > data_loop().
>
> I've realized the changes here are insufficient to handle the FD
> leaks.  That is, the accept()-ed FDs need to be closed inside the for
> loop so they aren't leaked with each iteration.  I'll re-work for a
> v2.

So it turns out this series doesn't leak FDs in the for loop.  FDs are
necessarily closed down in data_loop() when the read() returns 0.  The
only returns from data_loop() are after the FDs have been closed.
data_loop() and some of the functions it calls will call exit(1) on
error, but that won't leak FDs.

Please review this series.  Sorry for the confusion.

Regards,
Jason

Marek Marczykowski-Górecki June 13, 2020, 12:40 p.m. UTC | #3

On Wed, May 27, 2020 at 10:59:55PM -0400, Jason Andryuk wrote:
> On Mon, May 25, 2020 at 6:36 PM Jason Andryuk <[e-mail]> wrote:
> >
> > On Sun, May 24, 2020 at 10:50 PM Jason Andryuk <[e-mail]> wrote:
> > >
> > > This series addresses some Coverity reports.  To handle closing FDs, a
> > > state struct is introduced to track FDs closed in both main() and
> > > data_loop().
> >
> > I've realized the changes here are insufficient to handle the FD
> > leaks.  That is, the accept()-ed FDs need to be closed inside the for
> > loop so they aren't leaked with each iteration.  I'll re-work for a
> > v2.
> 
> So it turns out this series doesn't leak FDs in the for loop.  FDs are
> necessarily closed down in data_loop() when the read() returns 0.  The
> only returns from data_loop() are after the FDs have been closed.
> data_loop() and some of the functions it calls will call exit(1) on
> error, but that won't leak FDs.
> 
> Please review this series.  Sorry for the confusion.

For the whole series:

Reviewed-by: Marek Marczykowski-Górecki <[e-mail]>

Jason Andryuk June 14, 2020, 2:05 p.m. UTC | #4

On Sat, Jun 13, 2020 at 8:40 AM Marek Marczykowski-Górecki
<[e-mail]> wrote:
>
> On Wed, May 27, 2020 at 10:59:55PM -0400, Jason Andryuk wrote:
> > On Mon, May 25, 2020 at 6:36 PM Jason Andryuk <[e-mail]> wrote:
> > >
> > > On Sun, May 24, 2020 at 10:50 PM Jason Andryuk <[e-mail]> wrote:
> > > >
> > > > This series addresses some Coverity reports.  To handle closing FDs, a
> > > > state struct is introduced to track FDs closed in both main() and
> > > > data_loop().
> > >
> > > I've realized the changes here are insufficient to handle the FD
> > > leaks.  That is, the accept()-ed FDs need to be closed inside the for
> > > loop so they aren't leaked with each iteration.  I'll re-work for a
> > > v2.
> >
> > So it turns out this series doesn't leak FDs in the for loop.  FDs are
> > necessarily closed down in data_loop() when the read() returns 0.  The
> > only returns from data_loop() are after the FDs have been closed.
> > data_loop() and some of the functions it calls will call exit(1) on
> > error, but that won't leak FDs.
> >
> > Please review this series.  Sorry for the confusion.
>
> For the whole series:
>
> Reviewed-by: Marek Marczykowski-Górecki <[e-mail]>

Thanks for the review.  Sorry for forgetting to CC you on this series
and the v2 posted on Jun 10th as well.  For v2, patch 1 now also
changes strncpy to strcpy to avoid a gcc-10 warning reported by Olaf
Hering.  Patches 2 & 3 are new to move some perror calls.  v1 patches
2-8 shifted to 4-10 in v2, but are unchanged.

Thanks,
Jason