Re: [REQUEST FOR VOTES] snmpd: Call fclose() after fdopen() but not close()
Magnus Fromreide <[email protected]>
| Newsgroups | gmane.network.net-snmp.devel |
|---|---|
| Message-ID | <[email protected]> |
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
_______________________________________________
Net-snmp-coders mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
pass_persist-kill-fOut-fdIn.diff
(text/plain, 2.3 KB)
diff --git a/agent/mibgroup/ucd-snmp/pass_persist.c b/agent/mibgroup/ucd-snmp/pass_persist.c
index 173510aa51..f16894609a 100644
--- a/agent/mibgroup/ucd-snmp/pass_persist.c
+++ b/agent/mibgroup/ucd-snmp/pass_persist.c
@@ -48,8 +48,8 @@ netsnmp_feature_require(parse_miboid);
struct extensible *persistpassthrus = NULL;
int numpersistpassthrus = 0;
struct persist_pipe_type {
- FILE *fIn, *fOut;
- int fdIn, fdOut;
+ FILE *fIn;
+ int fdOut;
netsnmp_pid_t pid;
} *persist_pipes = (struct persist_pipe_type *) NULL;
static unsigned pipe_check_alarm_id;
@@ -521,8 +521,8 @@ init_persist_pipes(void)
(numpersistpassthrus + 1));
if (persist_pipes) {
for (i = 0; i <= numpersistpassthrus; i++) {
- persist_pipes[i].fIn = persist_pipes[i].fOut = (FILE *) 0;
- persist_pipes[i].fdIn = persist_pipes[i].fdOut = -1;
+ persist_pipes[i].fIn = (FILE *) 0;
+ persist_pipes[i].fdOut = -1;
persist_pipes[i].pid = NETSNMP_NO_SUCH_PROCESS;
}
}
@@ -623,15 +623,9 @@ open_persist_pipe(int iindex, char *command)
* If not, fill out our structure
*/
persist_pipes[iindex].pid = pid;
- persist_pipes[iindex].fdIn = fdIn;
persist_pipes[iindex].fdOut = fdOut;
persist_pipes[iindex].fIn = fdopen(fdIn, "r");
- persist_pipes[iindex].fOut = fdopen(fdOut, "w");
- /*
- * Setup our -non-buffered-io-
- */
- setbuf(persist_pipes[iindex].fOut, (char *) 0);
DEBUGMSGTL(("ucd-snmp/pass_persist", "open_persist_pipe: opened the pipes\n"));
}
@@ -771,16 +765,14 @@ close_persist_pipe(int iindex)
/*
* Check and nix every item
*/
- if (persist_pipes[iindex].fOut) {
- fclose(persist_pipes[iindex].fOut);
- persist_pipes[iindex].fOut = (FILE *) 0;
+ if (persist_pipes[iindex].fdOut != -1) {
+ close(persist_pipes[iindex].fdOut);
+ 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;
}
- persist_pipes[iindex].fdIn = -1;
#ifdef __uClinux__
/*remove the pipes*/