Re: [REQUEST FOR VOTES] snmpd: Call fclose() after fdopen() but not close()

[Magnus F. <ma...@ly...>]

On Sun, Aug 09, 2020 at 01:16:37AM +0200, Magnus Fromreide wrote:
> On Sat, Aug 08, 2020 at 11:17:52AM -0700, Bart Van Assche wrote:
> > From https://pubs.opengroup.org/onlinepubs/9699919799/functions/fclose.html:
> > "The fclose() function shall perform the equivalent of a close() on the file
> > descriptor that is associated with the stream pointed to by stream."
> > 
> > See also https://github.com/net-snmp/net-snmp/issues/157 .
> > 
> > Fixes: fd9a42d142d8 ("- (pass-persist.c pass-persist.h): moved to pass_persist.[ch].")
> > Fixes: a36188e50dcc ("Patch #760417 from Bob Rowlands/Sun for fixing Bug #751920")
> 
> That file is a mess.
> 
> Have anyone heard from Alexander Prömel regarding those temporary hard coded
> pathnames that should be fixed soon? I think 14 years cover any reasonable
> definition of the word 'soon'.
> 
> Why are both the file descriptor and the FILE* stored in the first place?
> It seems the read part of the code is using buffered I/O (fIn) while the
> write part of the code is using unbuffered I/O (fdOut) so I think fdIn and
> fOut should be removed completley.
> 
> Why is the SIGPIPE dance done when agent/snmpd explicitly ignores SIGPIPE?
> 
> I am putting in a 0 vote here because I think a larger rewrite is needed.

Now I have finally gotten around to look more throughly into this and found
that your patch is basically good.

For the agent/snmpd.c part I would like to change my vote to +1.

For the pass_persist part you are keeping the fclose of fOut but the rest of
the code is writing to fdOut so I would like to add the attached patch on top
of yours. It kills of fOut and fdIn so that there is only one reference to
the file descriptor.

One could note that the handling of a failed call to fdopen is bad in all
versions of the code except the original one but I think that is a smaller
problem than 'Does not work on windows' and 'Is not threadsafe'.

/MF