net-snmp-coders — Discussion of internal project coding (Please use this OR -users, NOT both!)

Re: [REQUEST FOR VOTES] snmpd: Drop support for pass_persist on uClinux

[Bart V. A. <bva...@ac...>]

On 2020-08-16 15:58, Wes Hardaker wrote:
> Magnus Fromreide <ma...@ly...> writes:
> 
>> That is a more lenient option than just dropping it so I think 'happy with
>> dropping it' covers this variant. On the other hand I usually am far from
>> the most conservative one in discussions like  this one.
> 
> I think off by default makes a lot more sense than an out-right removal.

How about restoring uClinux support with the patch below? parse_cmd() and
dup() are called before fork(). That change makes it possible to use a
regular pipe between the parent and the child, even when using vfork(). The
patch below has been tested on Linux (but not on uClinux).

Thanks,

Bart.


diff --git a/agent/mibgroup/util_funcs.c b/agent/mibgroup/util_funcs.c
index f39ea1adcd89..d42277b18f7e 100644
--- a/agent/mibgroup/util_funcs.c
+++ b/agent/mibgroup/util_funcs.c
@@ -458,53 +458,60 @@ int
 get_exec_pipes(const char *cmd, int *fdIn, int *fdOut, netsnmp_pid_t *pid)
 {
 #if defined(HAVE_EXECV)
-    int             fd[2][2];
+    int             fd[2][2], stdin_fd, stdout_fd, stderr_fd;
     char           **argv, *args;

+    argv = parse_cmd(&args, cmd);
+    if (!argv) {
+        DEBUGMSGTL(("util_funcs", "get_exec_pipes(): argv == NULL\n"));
+        return 0;
+    }
     /*
-     * Setup our pipes
+     * Setup pipes
      */
     if (pipe(fd[0]) || pipe(fd[1])) {
         setPerrorstatus("pipe");
         return 0;
     }
-    if ((*pid = fork()) == 0) { /* First handle for the child */
-        close(fd[0][1]);
-        close(fd[1][0]);
-        close(0);
-        if (dup(fd[0][0]) != 0) {
-            setPerrorstatus("dup 0");
-            return 0;
-        }
-        close(fd[0][0]);
-        close(1);
-        if (dup(fd[1][1]) != 1) {
-            setPerrorstatus("dup 1");
-            return 0;
-        }
-        close(fd[1][1]);
-
-        /*
-         * write standard output and standard error to pipe.
-         */
+    /* Save stdin, stdout and stderr */
+    stdin_fd = dup(STDIN_FILENO);
+    stdout_fd = dup(STDOUT_FILENO);
+    stderr_fd = dup(STDERR_FILENO);
+    /* Redirect stdin, stdout and stderr to the child side of the pipe */
+    if (dup2(fd[0][0], STDIN_FILENO) != 0) {
+        setPerrorstatus("dup 0");
+        return 0;
+    }
+    if (dup2(fd[1][1], STDOUT_FILENO) != 1) {
+        setPerrorstatus("dup 1");
+        return 0;
+    }
+    NETSNMP_IGNORE_RESULT(dup2(STDOUT_FILENO, STDERR_FILENO));
+#ifdef __uClinux__
+    if ((*pid = vfork()) == 0) {
+#else
+    if ((*pid = fork()) == 0) {
         /*
          * close all non-standard open file descriptors
          */
-        netsnmp_close_fds(1);
-        NETSNMP_IGNORE_RESULT(dup(1));  /* stderr */
+        netsnmp_close_fds(STDOUT_FILENO);
+#endif

-        argv = parse_cmd(&args, cmd);
-        if (!argv) {
-            DEBUGMSGTL(("util_funcs", "get_exec_pipes(): argv == NULL\n"));
-            return 0;
-        }
-        DEBUGMSGTL(("util_funcs", "get_exec_pipes(): argv[0] = %s\n", argv[0]));
+        /* Child process */
         execv(argv[0], argv);
         perror(argv[0]);
         free(argv);
         free(args);
-        exit(1);
+        return 1;
     } else {
+        /* Parent process. */
+        /* Restore stdin, stdout and stderr */
+        dup2(stdin_fd, STDIN_FILENO);
+        close(stdin_fd);
+        dup2(stdout_fd, STDOUT_FILENO);
+        close(stdout_fd);
+        dup2(stderr_fd, STDERR_FILENO);
+        close(stderr_fd);
         close(fd[0][0]);
         close(fd[1][1]);
         if (*pid < 0) {

Re: [REQUEST FOR VOTES] snmpd: Drop support for pass_persist on uClinux

[Wes H. <har...@us...>]

Magnus Fromreide <ma...@ly...> writes:

> That is a more lenient option than just dropping it so I think 'happy with
> dropping it' covers this variant. On the other hand I usually am far from
> the most conservative one in discussions like  this one.

I think off by default makes a lot more sense than an out-right removal.
-- 
Wes Hardaker
Please mail all replies to net...@li...

Re: net-snmp 5.9 released (early announcement)

[Andy F. <an...@om...>]

Just FYI, the archive contains some .o files.
This causes problems when building on a different architecture or 32-bit.

Andy

[REQUEST FOR VOTES] snmpd: Drop support for pass_persist on uClinux

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

On Sat, Aug 15, 2020 at 09:11:40AM -0700, Bart Van Assche wrote:
> On 2020-08-15 07:14, Magnus Fromreide wrote:
> > This allows vfork to not block the parent until the child has called _exit
> > or exec*, it does not allow the child to block the parent until completion.
> > 
> > Please note that in our case this is arch-specific code for uClinux where
> > the fork() system call doesn't exist so I think we can be assured that vfork
> > will block the parent there.
> 
> On https://www.eetimes.com/how-uclinux-provides-mmu-less-processors-with-an-alternative/#
> I found the following, which confirms the above:
> 
>   uClinux implements vfork() in order to compensate for the lack of fork().
>   When a parent process calls vfork() to create a child, both processes share
>   all their memory space including the stack. vfork() then suspends the
>   parent's execution until the child process either calls exit() or execve().
> 
> > If we had been religous about marking all internal fd's as CLOEXEC then
> > pass_perrsistant could be rewritten using posix_spawn which sounds a lot
> > better but we are sadly lacking in the CLOEXEC department.
> > 
> > To be honest the calls to perror and exit if exec fails is more suspicious as
> > that is explicitly stated to cause undefined behaviour.
> > 
> > With all this I should probably repeat that I am in favour of dropping the
> > pass_persist support for uClinux.
> 
> How about removing the pass_persist for uClinux and restoring it if a
> user asks to restore that support? uClinux was created in 1998, when
> the price difference between microprocessors with or without MMU was
> much larger than today. Today a Raspberry Pi is available for $35. That
> product includes not only a CPU with MMU but also a board and connectors.

That is a more lenient option than just dropping it so I think 'happy with
dropping it' covers this variant. On the other hand I usually am far from
the most conservative one in discussions like  this one.

/MF

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

[Bart V. A. <bva...@ac...>]

On 2020-08-15 07:14, Magnus Fromreide wrote:
> This allows vfork to not block the parent until the child has called _exit
> or exec*, it does not allow the child to block the parent until completion.
> 
> Please note that in our case this is arch-specific code for uClinux where
> the fork() system call doesn't exist so I think we can be assured that vfork
> will block the parent there.

On https://www.eetimes.com/how-uclinux-provides-mmu-less-processors-with-an-alternative/#
I found the following, which confirms the above:

  uClinux implements vfork() in order to compensate for the lack of fork().
  When a parent process calls vfork() to create a child, both processes share
  all their memory space including the stack. vfork() then suspends the
  parent's execution until the child process either calls exit() or execve().

> If we had been religous about marking all internal fd's as CLOEXEC then
> pass_perrsistant could be rewritten using posix_spawn which sounds a lot
> better but we are sadly lacking in the CLOEXEC department.
> 
> To be honest the calls to perror and exit if exec fails is more suspicious as
> that is explicitly stated to cause undefined behaviour.
> 
> With all this I should probably repeat that I am in favour of dropping the
> pass_persist support for uClinux.

How about removing the pass_persist for uClinux and restoring it if a
user asks to restore that support? uClinux was created in 1998, when
the price difference between microprocessors with or without MMU was
much larger than today. Today a Raspberry Pi is available for $35. That
product includes not only a CPU with MMU but also a board and connectors.

Bart.

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

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

On Fri, Aug 14, 2020 at 07:38:09PM -0700, Bart Van Assche wrote:
> On 2020-08-14 17:05, Magnus Fromreide wrote:
> > The vfork child only blocks the parent until it have performed an successful
> > exec so that part looks ok [ ... ]
> 
> Hi Magnus,
> 
> Are you sure of this? From the Linux vfork() man page
> (https://man7.org/linux/man-pages/man2/vfork.2.html):
> 
> The requirements put on vfork() by the standards are weaker than
> those put on fork(2), so an implementation where the two are
> synonymous is compliant.  In particular, the programmer cannot rely
> on the parent remaining blocked until the child either terminates or
> calls execve(2), and cannot rely on any specific behavior with
> respect to shared memory.

This allows vfork to not block the parent until the child has called _exit
or exec*, it does not allow the child to block the parent until completion.

Please note that in our case this is arch-specific code for uClinux where
the fork() system call doesn't exist so I think we can be assured that vfork
will block the parent there.

If we had been religous about marking all internal fd's as CLOEXEC then
pass_perrsistant could be rewritten using posix_spawn which sounds a lot
better but we are sadly lacking in the CLOEXEC department.

To be honest the calls to perror and exit if exec fails is more suspicious as
that is explicitly stated to cause undefined behaviour.

With all this I should probably repeat that I am in favour of dropping the
pass_persist support for uClinux.

/MF

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

[Bart V. A. <bva...@ac...>]

On 2020-08-14 17:05, Magnus Fromreide wrote:
> The vfork child only blocks the parent until it have performed an successful
> exec so that part looks ok [ ... ]

Hi Magnus,

Are you sure of this? From the Linux vfork() man page
(https://man7.org/linux/man-pages/man2/vfork.2.html):

The requirements put on vfork() by the standards are weaker than
those put on fork(2), so an implementation where the two are
synonymous is compliant.  In particular, the programmer cannot rely
on the parent remaining blocked until the child either terminates or
calls execve(2), and cannot rely on any specific behavior with
respect to shared memory.

Thanks,

Bart.

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

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

On Thu, Aug 13, 2020 at 08:02:59PM -0700, Bart Van Assche wrote:
> On 2020-08-08 16:16, Magnus Fromreide wrote:
> > 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.
> 
> I propose to delete the uClinux code from agent/mibgroup/util_funcs.c.
> get_exec_pipes() is only used to open persistent pipes. So it is essential
> to use fork(). Using vfork() in the implementation of get_exec_pipes() is
> wrong because vfork() blocks the caller until the child process has
> finished.

The vfork child only blocks the parent until it have performed an successful
exec so that part looks ok but the fifo setup in the uClinux part looks very
magical, the only way I can see that code working is if /flash/cp_<pid> and
/flash/pc_<pid> somehow refer to the child processes stdin and stdout, and
even then it gives me an uneasy feeling of there beeing a race condition.

It is also worth noting that the argument splitting is missign from the
uClinux parts.

Given this I would agree with getting rid of the uClinux parts in
get_exec_pipes and pass_persist.

I suppose we should issue a deprecation notice on the -annonce list rather
soon even if I don't expect that anyone interested will read it.

If we decide to try to support pass_persist on uClinux in the future then I
think the right way forward would be to mark all internal file descriptors as
CLOEXEC and use posix_spawn.

/MF

net-snmp 5.9 released (early announcement)

[Wes H. <har...@us...>]

Folks,

After far too long, I've just published Net-SNMP 5.9.  I've created a
new V5-9-patches branch as well.

A full announcement will come early next week once I'm sure nothing is
wrong with the published files.  Along with a Net-SNMP sticker design
contest, with free stickers for anyone that has ever contributed code to
the Net-SNMP package before.  Stay tuned.

https://sourceforge.net/projects/net-snmp/files/net-snmp/5.9/

-- 
Wes Hardaker
Please mail all replies to net...@li...

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

[Bart V. A. <bva...@ac...>]

On 2020-08-08 16:16, Magnus Fromreide wrote:
> 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.

I propose to delete the uClinux code from agent/mibgroup/util_funcs.c.
get_exec_pipes() is only used to open persistent pipes. So it is essential
to use fork(). Using vfork() in the implementation of get_exec_pipes() is
wrong because vfork() blocks the caller until the child process has
finished.

Bart.

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

[Bart V. A. <bva...@ac...>]

On 2020-08-13 18:16, Magnus Fromreide wrote:
> Now I have finally gotten around to look more thoroughly 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.

Thanks!

> 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. 
The attached patch looks good to me.

Bart.

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

Re: Test failures on V5-8-patches and master

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

On Thu, Aug 13, 2020 at 03:55:31PM -0700, Wes Hardaker via Net-snmp-coders wrote:
> Magnus Fromreide <ma...@ly...> writes:
> 
> > I just noticed that T0222snmpv3bulkget_simple depends on SHA authentication
> > and AES privacy but fails to check for this so if one builds the agent
> > without that support the test fails.
> 
> Is it really just that test?  I actually thought that most of the tests
> were using at least sha for authentication.  (no I didn't go look).

That was the only one that FAILED rather than getting skipped when I built
with --with-openssl=internal --disable-privacy.

> In my memory we wrote most of them assuming that snmpv3 support was
> going to be used for most of the tests, as it was the most complex of
> the protocol options.  And few people would turn off v3.

That is a separate issue, you can't turn off v3 even if you want to so it is
always there.

> > Now, I am not certain about exactly what T0222 tests but I think it is about
> > bulkget limits and thus the use of encryption and privacy is just a
> > distraction so I suggest that $PRIVTESTARGS is replaced with $NOAUTHTESTARGS.
> 
> I'd rather see $AUTHTESTARGS, which is in most other places.

Fair enough, I was basing the change on T0220 and T0221 who are both using
NOAUTHTESTARGS.

Looking at T024* seems to suggest that AUTHTESTARGS require some SKIPIF's
though.

/MF

Re: Test failures on V5-8-patches and master

[Wes H. <har...@us...>]

Magnus Fromreide <ma...@ly...> writes:

> I just noticed that T0222snmpv3bulkget_simple depends on SHA authentication
> and AES privacy but fails to check for this so if one builds the agent
> without that support the test fails.

Is it really just that test?  I actually thought that most of the tests
were using at least sha for authentication.  (no I didn't go look).

In my memory we wrote most of them assuming that snmpv3 support was
going to be used for most of the tests, as it was the most complex of
the protocol options.  And few people would turn off v3.

> Now, I am not certain about exactly what T0222 tests but I think it is about
> bulkget limits and thus the use of encryption and privacy is just a
> distraction so I suggest that $PRIVTESTARGS is replaced with $NOAUTHTESTARGS.

I'd rather see $AUTHTESTARGS, which is in most other places.
-- 
Wes Hardaker
Please mail all replies to net...@li...

Test failures on V5-8-patches and master

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

Hello!

I just noticed that T0222snmpv3bulkget_simple depends on SHA authentication
and AES privacy but fails to check for this so if one builds the agent
without that support the test fails.

Now, I am not certain about exactly what T0222 tests but I think it is about
bulkget limits and thus the use of encryption and privacy is just a
distraction so I suggest that $PRIVTESTARGS is replaced with $NOAUTHTESTARGS.

If the encryption is important then the alternative is to add

SKIPIFNOT NETSNMP_CAN_DO_CRYPTO
SKIPIFNOT NETSNMP_ENABLE_SCAPI_AUTHPRIV
SKIPIFNOT HAVE_AES_CFB128_ENCRYPT

in order to skip the test when that prerequisite ain't available.

/MF

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

[Wes H. <har...@us...>]

Bart Van Assche <bva...@ac...> writes:

> My understanding is that the crashes triggered by the double close() calls
> only occur on Windows systems and also that these crashes were fixed a long
> time ago. For Unix systems this patch fixes the race condition described in
> https://github.com/net-snmp/net-snmp/issues/157.

So, this one looks on the fence to me since it's fairly complex overall
to analyze fully and we're so deep into the release.  I haven't made up
my own mind, but it doesn't look like it'll get the votes needed at
this point to put it in.  I suggest we put it up as an official patch
instead.
-- 
Wes Hardaker
Please mail all replies to net...@li...

Fw: Newly built 5.8 on QNX 7

[Ian C <mc...@ya...>]

 Could my dlist sigabrt issue be the same ashttps://github.com/net-snmp/net-snmp/issues/109 
??
Ian




   ----- Forwarded Message ----- From: Ian C via Net-snmp-coders <net...@li...>To: net...@li... <net...@li...>Sent: Tuesday, August 11, 2020, 4:37:00 p.m. EDTSubject: Fw: Newly built 5.8 on QNX 7
 Thanks to Wes & Aferjes, I did some further investigating regarding my table subagent null ptr on the table registration - it turned out that in our upgrade from 5.6 to 5.8 we were pulling in headers from 5.6 but linking against 5.8!  Oops.
Now, our agent can register:
handler:inject: injecting table before table_iterator
trace: netsnmp_register_handler(): agent_handler.c, 269:
handler::register: Registering chAsinHwSbcNtpTable (::table::table_iterator::chAsinHwSbcNtpTable) at iso.3.6.1.4.1.25228.3.3.1.9.6.6
trace: netsnmp_inject_handler_before(): agent_handler.c, 444:
handler:inject: injecting bulk_to_next before table
trace: netsnmp_register_mib(): agent_registry.c, 1155:
register_mib: registering "chAsinHwSbcNtpTable" at iso.3.6.1.4.1.25228.3.3.1.9.6.6 with context "(null)"
 However it crashes after the 1st query:chAsinHwSbcNtpTable: Processing request (160)
trace: netsnmp_call_handler(): agent_handler.c, 549:
handler:returned: handler chAsinHwSbcNtpTable returned 0
trace: netsnmp_call_handler(): agent_handler.c, 549:
handler:returned: handler table_iterator returned 0
trace: netsnmp_call_handler(): agent_handler.c, 549:
handler:returned: handler table returned 0
trace: handle_getnext_loop(): snmp_agent.c, 3499:
results:summary: gathered 0/2 varbinds
trace: netsnmp_handle_request(): snmp_agent.c, 3684:
results: request results (status = 0):
trace: netsnmp_handle_request(): snmp_agent.c, 3687:
results:        trace: sprint_realloc_by_type(): mib.c, 2032:
output: sprint_by_type, type 4
iso.3.6.1.4.1.25228.3.3.1.9.6.6.1.1.10.65.49.0.0.0.0.0.0.0.0 = STRING: "A1"
trace: netsnmp_handle_request(): snmp_agent.c, 3687:
results:        trace: sprint_realloc_by_type(): mib.c, 2032:
output: sprint_by_type, type 4
iso.3.6.1.4.1.25228.3.3.1.9.6.6.1.2.10.65.49.0.0.0.0.0.0.0.0 = STRING: "{\"data\":[{... JSON DATA DELETED ... SNIP! ..."}]}"
trace: netsnmp_callback_hook_build(): transports/snmpCallbackDomain.c, 460:
transport_callback: hook_build enter
trace: netsnmp_callback_hook_build(): transports/snmpCallbackDomain.c, 534:
transport_callback: hook_build exit
trace: _build_initial_pdu_packet(): snmp_api.c, 5151:
sess_async_send: final pktbuf_len after building packet 484
trace: _sess_async_send(): snmp_api.c, 5288:
sess_process_packet: sending message id#0 reqid#1419 len 1
transport:send: 1 bytes to callback: 1 on fd 3
trace: netsnmp_callback_send(): transports/snmpCallbackDomain.c, 241:
transport_callback: hook_send enter
trace: callback_debug_pdu(): transports/snmpCallbackDomain.c, 94:
dump_send_callback_transport: PDU: command = 162, errstat = 0, errindex = 0
trace: callback_debug_pdu(): transports/snmpCallbackDomain.c, 96:
dump_send_callback_transport:   var 2:trace: sprint_realloc_by_type(): mib.c, 2032:
output: sprint_by_type, type 4
iso.3.6.1.4.1.25228.3.3.1.9.6.6.1.1.10.65.49.0.0.0.0.0.0.0.0 = STRING: "A1"
trace: callback_debug_pdu(): transports/snmpCallbackDomain.c, 96:
dump_send_callback_transport:   var 4:trace: sprint_realloc_by_type(): mib.c, 2032:
output: sprint_by_type, type 4
iso.3.6.1.4.1.25228.3.3.1.9.6.6.1.2.10.65.49.0.0.0.0.0.0.0.0 = STRING: "{\"data\":[{ ... JSON DATA DELETED ... SNIP! ..."}]}"
trace: netsnmp_callback_send(): transports/snmpCallbackDomain.c, 322:
transport_callback: hook_send exit
trace: netsnmp_remove_and_free_agent_snmp_session(): snmp_agent.c, 2128:
snmp_agent: REMOVE session ==  806e0a8
trace: free_agent_snmp_session(): snmp_agent.c, 1601:
snmp_agent: agent_session  806e0a8 released
trace: free_agent_snmp_session(): snmp_agent.c, 1606:
verbose:asp: asp 806e0a8 reqinfo 80721b8 freed
free malloc object that is not allocated:/builds/workspace/BC700_6762_sdp704/build_dir/lib/c/alloc/dlist.c:1129
ABORT HANDLER signal: 6

If anyone has any theoughts, as usual, they'd be greatly appreciated.
Cheers,Ian
_______________________________________________
Net-snmp-coders mailing list
Net...@li...
https://lists.sourceforge.net/lists/listinfo/net-snmp-coders
  

Fw: Newly built 5.8 on QNX 7

[Ian C <mc...@ya...>]

Thanks to Wes & Aferjes, I did some further investigating regarding my table subagent null ptr on the table registration - it turned out that in our upgrade from 5.6 to 5.8 we were pulling in headers from 5.6 but linking against 5.8!  Oops.
Now, our agent can register:
handler:inject: injecting table before table_iterator
trace: netsnmp_register_handler(): agent_handler.c, 269:
handler::register: Registering chAsinHwSbcNtpTable (::table::table_iterator::chAsinHwSbcNtpTable) at iso.3.6.1.4.1.25228.3.3.1.9.6.6
trace: netsnmp_inject_handler_before(): agent_handler.c, 444:
handler:inject: injecting bulk_to_next before table
trace: netsnmp_register_mib(): agent_registry.c, 1155:
register_mib: registering "chAsinHwSbcNtpTable" at iso.3.6.1.4.1.25228.3.3.1.9.6.6 with context "(null)"
 However it crashes after the 1st query:chAsinHwSbcNtpTable: Processing request (160)
trace: netsnmp_call_handler(): agent_handler.c, 549:
handler:returned: handler chAsinHwSbcNtpTable returned 0
trace: netsnmp_call_handler(): agent_handler.c, 549:
handler:returned: handler table_iterator returned 0
trace: netsnmp_call_handler(): agent_handler.c, 549:
handler:returned: handler table returned 0
trace: handle_getnext_loop(): snmp_agent.c, 3499:
results:summary: gathered 0/2 varbinds
trace: netsnmp_handle_request(): snmp_agent.c, 3684:
results: request results (status = 0):
trace: netsnmp_handle_request(): snmp_agent.c, 3687:
results:        trace: sprint_realloc_by_type(): mib.c, 2032:
output: sprint_by_type, type 4
iso.3.6.1.4.1.25228.3.3.1.9.6.6.1.1.10.65.49.0.0.0.0.0.0.0.0 = STRING: "A1"
trace: netsnmp_handle_request(): snmp_agent.c, 3687:
results:        trace: sprint_realloc_by_type(): mib.c, 2032:
output: sprint_by_type, type 4
iso.3.6.1.4.1.25228.3.3.1.9.6.6.1.2.10.65.49.0.0.0.0.0.0.0.0 = STRING: "{\"data\":[{... JSON DATA DELETED ... SNIP! ..."}]}"
trace: netsnmp_callback_hook_build(): transports/snmpCallbackDomain.c, 460:
transport_callback: hook_build enter
trace: netsnmp_callback_hook_build(): transports/snmpCallbackDomain.c, 534:
transport_callback: hook_build exit
trace: _build_initial_pdu_packet(): snmp_api.c, 5151:
sess_async_send: final pktbuf_len after building packet 484
trace: _sess_async_send(): snmp_api.c, 5288:
sess_process_packet: sending message id#0 reqid#1419 len 1
transport:send: 1 bytes to callback: 1 on fd 3
trace: netsnmp_callback_send(): transports/snmpCallbackDomain.c, 241:
transport_callback: hook_send enter
trace: callback_debug_pdu(): transports/snmpCallbackDomain.c, 94:
dump_send_callback_transport: PDU: command = 162, errstat = 0, errindex = 0
trace: callback_debug_pdu(): transports/snmpCallbackDomain.c, 96:
dump_send_callback_transport:   var 2:trace: sprint_realloc_by_type(): mib.c, 2032:
output: sprint_by_type, type 4
iso.3.6.1.4.1.25228.3.3.1.9.6.6.1.1.10.65.49.0.0.0.0.0.0.0.0 = STRING: "A1"
trace: callback_debug_pdu(): transports/snmpCallbackDomain.c, 96:
dump_send_callback_transport:   var 4:trace: sprint_realloc_by_type(): mib.c, 2032:
output: sprint_by_type, type 4
iso.3.6.1.4.1.25228.3.3.1.9.6.6.1.2.10.65.49.0.0.0.0.0.0.0.0 = STRING: "{\"data\":[{ ... JSON DATA DELETED ... SNIP! ..."}]}"
trace: netsnmp_callback_send(): transports/snmpCallbackDomain.c, 322:
transport_callback: hook_send exit
trace: netsnmp_remove_and_free_agent_snmp_session(): snmp_agent.c, 2128:
snmp_agent: REMOVE session ==  806e0a8
trace: free_agent_snmp_session(): snmp_agent.c, 1601:
snmp_agent: agent_session  806e0a8 released
trace: free_agent_snmp_session(): snmp_agent.c, 1606:
verbose:asp: asp 806e0a8 reqinfo 80721b8 freed
free malloc object that is not allocated:/builds/workspace/BC700_6762_sdp704/build_dir/lib/c/alloc/dlist.c:1129
ABORT HANDLER signal: 6

If anyone has any theoughts, as usual, they'd be greatly appreciated.
Cheers,Ian

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

[Bart V. A. <bva...@ac...>]

On 2020-08-10 10:51, Wes Hardaker wrote:
> Bart Van Assche <bva...@ac...> writes:
> 
>> That's a great question. This fix only affects pass-persist on Unix
>> platforms and Cygwin but neither affects the MSVC build nor any of the
>> MinGW builds since the affected code was already disabled for the
>> latter two platforms.
> 
> Thanks.  Follow-on: do we have any clue which OSes actually crash as a
> result of the double close?  (which I admit is something that should
> never have been done)

Hi Wes,

My understanding is that the crashes triggered by the double close() calls
only occur on Windows systems and also that these crashes were fixed a long
time ago. For Unix systems this patch fixes the race condition described in
https://github.com/net-snmp/net-snmp/issues/157.

Bart.

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

[Wes H. <har...@us...>]

Bart Van Assche <bva...@ac...> writes:

> That's a great question. This fix only affects pass-persist on Unix
> platforms and Cygwin but neither affects the MSVC build nor any of the
> MinGW builds since the affected code was already disabled for the
> latter two platforms.

Thanks.  Follow-on: do we have any clue which OSes actually crash as a
result of the double close?  (which I admit is something that should
never have been done)
-- 
Wes Hardaker
Please mail all replies to net...@li...

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

[Bart V. A. <bva...@ac...>]

On 2020-08-10 08:37, Wes Hardaker wrote:
> Bart Van Assche <bva...@ac...> writes:
> 
>> However, I don't think rewriting that file so close to the v5.9 release
>> is appropriate. Hence this patch that makes the smallest possible
>> change.
> 
> To be clear, this affects *only* pass-persist *only* on windows correct?
> IE, it's a fix for a specific combination.  But the fix may affect many
> other platforms if the fix itself contains a bug?

Hi Wes,

That's a great question. This fix only affects pass-persist on Unix
platforms and Cygwin but neither affects the MSVC build nor any of the
MinGW builds since the affected code was already disabled for the
latter two platforms.

Thanks,

Bart.

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

[Wes H. <har...@us...>]

Bart Van Assche <bva...@ac...> writes:

> However, I don't think rewriting that file so close to the v5.9 release
> is appropriate. Hence this patch that makes the smallest possible
> change.

To be clear, this affects *only* pass-persist *only* on windows correct?
IE, it's a fix for a specific combination.  But the fix may affect many
other platforms if the fix itself contains a bug?
-- 
Wes Hardaker
Please mail all replies to net...@li...

snmptrapd

[jayshankar n. <jay...@gm...>]

Hi,

I am testing snmptrapd daemon. snmptrapd is not processing traps from
certain ip address. I am receiving the traps in tcpdump. My linux release
is Centos 7. I have disable the firewalld. iptables is also not running.

Appreciate your help.

Thanks,
Jayshankar

snmptrapd Logs:
 snmptrapd -c /tmp/snmptrapd.conf  -f -Le
NET-SNMP version 5.7.1
2020-08-09 06:46:54 <UNKNOWN> [UDP:
[172.31.3.186]:55965->[172.31.21.53]:162]:
DISMAN-EVENT-MIB::sysUpTimeInstance = Timeticks: (1218563900) 141 days,
0:53:59.00      SNMPv2-MIB::snmpTrapOID.0 = OID:
SNMPv2-SMI::enterprises.14823.2.3.1.11.1.2.1195
 SNMPv2-SMI::enterprises.14823.2.3.1.11.1.1.60 = Hex-STRING: 07 4 08 09 15
2D 39 00 2B 05 1E      SNMPv2-SMI::enterprises.14823.2.3.1.11.1.1.5.0 =
Hex-STRING: 10 07 B6 08 43 66
 SNMPv2-SMI::enterprises.14823.2.3.1.11.1.1.6.0 = ""
SNMPv2-SMI::enterprises.14823.2.3.1.11.1.1.18.0 = INTEGER: 6
2020-08-09 06:46:56 <UNKNOWN> [UDP:
[172.31.3.186]:55965->[172.31.21.53]:162]:
DISMAN-EVENT-MIB::sysUpTimeInstance = Timeticks: (1218564100) 141 days,
0:54:01.00      SNMPv2-MIB::snmpTrapOID.0 = OID:
SNMPv2-SMI::enterprises.14823.2.3.1.11.1.2.1195
 SNMPv2-SMI::enterprises.14823.2.3.1.11.1.1.60 = Hex-STRING: 07 4 08 09 15
2D 3A 00 2 05 1E      SNMPv2-SMI::enterprises.14823.2.3.1.11.1.1.5.0 =
Hex-STRING: 2 6F C9 08 BB E1
 SNMPv2-SMI::enterprises.14823.2.3.1.11.1.1.6.0 = STRING: "JioFi3_08BBE1"
SNMPv2-SMI::enterprises.14823.2.3.1.11.1.1.18.0 = INTEGER: 11

tcpdump log.(Not receiving below trap)
6:48:44.569504 IP 172.31.9.170.54046 > localhost.localdomain.snmptrap:
 C="Ra" V2Trap(259)  system.sysUpTime.0=1286370600
S:1.1.4.1.0=E:cisco.9.599.0.4  E:cisco.9.599.1.3.1.1.1.0=a4_4b_d5_4e
E:cisco.9.513.1.1.1.1.5.0="AP-01-02" E:cisco.9.599.1.3.1.1.8.0=7
E:cisco.9.513.1.2.1.1.1.0=0 E:cisco.9.599.1.3.1.1.10.0=100
E:cisco.9.599.1.3.1.1.27.0="" E:cisco.9.599.1.3.1.1.28.0="R"

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

[Bart V. A. <bva...@ac...>]

On 2020-08-08 16:16, Magnus Fromreide wrote:
> I am putting in a 0 vote here because I think a larger rewrite is needed.

Hi Magnus,

I agree that agent/mibgroup/ucd-snmp/pass_persist.c should be rewritten.
However, I don't think rewriting that file so close to the v5.9 release
is appropriate. Hence this patch that makes the smallest possible change.

Thanks,

Bart.

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

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

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
> Net...@li...
> https://lists.sourceforge.net/lists/listinfo/net-snmp-coders