snmp: fix dangling pointer in snmp_traps

[j123b567]

fixes: https://savannah.nongnu.org/bugs/?64685
fixes: https://savannah.nongnu.org/bugs/?64847
replaces: #42 which is wrong
replaces: #41 which is duplicate of #42

[yarrick]

With this contrib/ports/unix/example_app still fails to compile on my machine (using GCC 14.1):

cc -g -DLWIP_DEBUG -Wall -pedantic -Werror -Wparentheses -Wsequence-point -Wswitch-default -Wextra -Wundef -Wshadow -Wpointer-arith -Wcast-qual -Wc++-compat -Wwrite-strings -Wold-style-definition -Wcast-align -Wmissing-prototypes -Wnested-externs -Wunreachable-code -Wuninitialized -Wmissing-prototypes -Wredundant-decls -Waggregate-return -Wlogical-not-parentheses -Wlogical-op -Wc90-c99-compat -Wtrampolines -I. -I../../.. -I../../../../src/include -I../../../ports/unix/port/include -I../../../examples/example_app  -c ../../../../src/apps/snmp/snmp_traps.c
../../../../src/apps/snmp/snmp_traps.c: In function ‘snmp_send_trap_or_notification_or_inform_generic’:
../../../../src/apps/snmp/snmp_traps.c:401:24: error: storing the address of local variable ‘snmp_v2_special_varbinds’ in ‘*varbinds.prev’ [-Werror=dangling-pointer=]
  401 |         varbinds->prev = &snmp_v2_special_varbinds[1];
      |         ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../src/apps/snmp/snmp_traps.c:357:23: note: ‘snmp_v2_special_varbinds’ declared here
  357 |   struct snmp_varbind snmp_v2_special_varbinds[] = {
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~
../../../../src/apps/snmp/snmp_traps.c:347:176: note: ‘varbinds’ declared here
  347 | uct snmp_msg_trap *trap_msg, const struct snmp_obj_id *eoid, s32_t generic_trap, s32_t specific_trap, struct snmp_varbind *varbinds)
      |                                                                                                       ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~

cc1: all warnings being treated as errors

[j123b567]

This function is a trap :(

This is not a real dangling pointer, because it is fixed afterwards, but the compiler is not smart enough to see it.

original_varbinds->prev = original_prev;

And I understand, because the code is terrible.

There is a much bigger problem with the SNMP code. snmp_varbind is defined with prev but it is never used in any part of SNMP. Only the snmp_trap code tries to set it correctly, but all traversal functions use only next.

Is it ok for you to remove all prev handling to prevent this situation?

This function had two dangling pointers. One was real and is fixed by the first commit, the second was false positive and is fixed by the second commit.

[yarrick]

Can you remove the prev pointer in struct snmp_varbind also to make sure it isn't used?

[j123b567]

Sure.

I thing, that it is breaking change for user applications. Nobody is probably using prev field but everybody is setting it to NULL during initialization of snmp_varbind.

[j123b567]

Should I also add something like this to UPGRADING?

  * The prev field in the snmp_varbind structure has been removed.

[yarrick]

I can add it for you.