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

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.

/MF

> ---
>  agent/mibgroup/ucd-snmp/pass_persist.c | 24 ++----------------------
>  agent/snmpd.c                          |  6 +-----
>  2 files changed, 3 insertions(+), 27 deletions(-)
> 
> diff --git a/agent/mibgroup/ucd-snmp/pass_persist.c b/agent/mibgroup/ucd-snmp/pass_persist.c
> index 4d88ff2b54bf..173510aa51c5 100644
> --- a/agent/mibgroup/ucd-snmp/pass_persist.c
> +++ b/agent/mibgroup/ucd-snmp/pass_persist.c
> @@ -775,32 +775,12 @@ close_persist_pipe(int iindex)
>          fclose(persist_pipes[iindex].fOut);
>          persist_pipes[iindex].fOut = (FILE *) 0;
>      }
> -    if (persist_pipes[iindex].fdOut != -1) {
> -#ifndef WIN32
> -        /*
> -         * The sequence open()/fdopen()/fclose()/close() triggers an access
> -         * violation with the MSVC runtime. Hence skip the close() call when
> -         * using the MSVC runtime.
> -         */
> -        close(persist_pipes[iindex].fdOut);
> -#endif
> -        persist_pipes[iindex].fdOut = -1;
> -    }
> +    persist_pipes[iindex].fdOut = -1;
>      if (persist_pipes[iindex].fIn) {
>          fclose(persist_pipes[iindex].fIn);
>          persist_pipes[iindex].fIn = (FILE *) 0;
>      }
> -    if (persist_pipes[iindex].fdIn != -1) {
> -#ifndef WIN32
> -        /*
> -         * The sequence open()/fdopen()/fclose()/close() triggers an access
> -         * violation with the MSVC runtime. Hence skip the close() call when
> -         * using the MSVC runtime.
> -         */
> -        close(persist_pipes[iindex].fdIn);
> -#endif
> -        persist_pipes[iindex].fdIn = -1;
> -    }
> +    persist_pipes[iindex].fdIn = -1;
>  
>  #ifdef __uClinux__
>  	/*remove the pipes*/
> diff --git a/agent/snmpd.c b/agent/snmpd.c
> index ae73eda1390e..2c925d8ff408 100644
> --- a/agent/snmpd.c
> +++ b/agent/snmpd.c
> @@ -955,17 +955,13 @@ main(int argc, char *argv[])
>              }
>          } else {
>              if ((PID = fdopen(fd, "w")) == NULL) {
> +                close(fd);
>                  snmp_log_perror(pid_file);
>                  goto out;
>              } else {
>                  fprintf(PID, "%d\n", (int) getpid());
>                  fclose(PID);
>              }
> -#ifndef _MSC_VER
> -            /* The sequence open()/fdopen()/fclose()/close() makes MSVC crash,
> -               hence skip the close() call when using the MSVC runtime. */
> -            close(fd);
> -#endif
>          }
>      }
>  #endif
> 
> 
> _______________________________________________
> Net-snmp-coders mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/net-snmp-coders