SNMP: Improve parsing of OIDs containing IP addresses (#2419)

Broken since 2007 commit cc192b50 made a false "4 or 16" assumption,
possibly influenced by bad assumptions in earlier client_Inst() code.

Also improved const-correctness to get fixed oid2addr() code to compile.

diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index 8e70676885f708ad169d398c4b9fd9f57701f757..4509413cee344a120ca72d198f6336ccec45dbcc 100644 (file)
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -439,7 +439,6 @@ Thank you!
     Regents of the University of California (UCSD)
     Reinhard Posmyk <Reinhard.Posmyk@arxes.de>
     Reinhard Sojka <reinhard.sojka@parlament.gv.at>
-    Renaud Metrich <renaud.metrich@gmail.com>
     Rene Geile <rene.geile@t-online.de>
     Reuben Farrelly <reuben@reub.net>
     Ricardo Ferreira Ribeiro <garb12@pm.me>

diff --git a/src/client_db.cc b/src/client_db.cc
index 21795607ab85eabbaed1b5d859c69a9d46f84d67..385befc84cfe7acfdfbdf794456185431bf10745 100644 (file)
--- a/src/client_db.cc
+++ b/src/client_db.cc
@@ -417,8 +417,8 @@ clientdbStartGC(void)
 
 #if SQUID_SNMP
 
-Ip::Address *
-client_entry(Ip::Address *current)
+const Ip::Address *
+client_entry(const Ip::Address *current)
 {
     char key[MAX_IPSTRLEN];
     hash_first(client_table);
@@ -449,9 +449,9 @@ snmp_meshCtblFn(variable_list * Var, snint * ErrP)
     MemBuf tmp;
     debugs(49, 6, "Current : length=" << Var->name_length << ": " << snmpDebugOid(Var->name, Var->name_length, tmp));
     if (Var->name_length == 16) {
-        oid2addr(&(Var->name[12]), keyIp, 4);
+        keyIp = oid2addr(&(Var->name[12]), 4).value();
     } else if (Var->name_length == 28) {
-        oid2addr(&(Var->name[12]), keyIp, 16);
+        keyIp = oid2addr(&(Var->name[12]), 16).value();
     } else {
         *ErrP = SNMP_ERR_NOSUCHNAME;
         return nullptr;

diff --git a/src/client_db.h b/src/client_db.h
index 24b782b563c191643198ee2eb5759a0f8ce69d77..85ee6cb4467993a0d9ac1704656afeabdb74e518 100644 (file)
--- a/src/client_db.h
+++ b/src/client_db.h
@@ -38,7 +38,7 @@ ClientInfo * clientdbGetInfo(const Ip::Address &addr);
 #endif
 
 #if SQUID_SNMP
-Ip::Address *client_entry(Ip::Address *current);
+const Ip::Address *client_entry(const Ip::Address *);
 variable_list *snmp_meshCtblFn(variable_list *, snint *);
 #endif
 

diff --git a/src/redirect.cc b/src/redirect.cc
index 528fed105dcca00be845b4f8cf37cebfb018b5c3..14a102714772c39c6b1f1bb2b35680bf3c5cc2ac 100644 (file)
--- a/src/redirect.cc
+++ b/src/redirect.cc
@@ -110,9 +110,8 @@ redirectHandleReply(void *data, const Helper::Reply &reply)
             // parse it into status=, url= and rewrite-url= keys
             if (replySize) {
                 MemBuf replyBuffer;
-                replyBuffer.init(replySize + 1, replySize + 1); // with space for 0-terminator added by append()
-                Assure(replySize <= size_t(reply.other().contentSize()));
-                replyBuffer.append(reply.other().content(), replySize);
+                replyBuffer.init(replySize, replySize);
+                replyBuffer.append(reply.other().content(), reply.other().contentSize());
                 char * result = replyBuffer.content();
 
                 Helper::Reply newReply;

diff --git a/src/snmp_core.cc b/src/snmp_core.cc
index 4e06c250ae9b3040235ac04202fd8405784945aa..4b91a491ff59868663988fc4bd8d7e49731c4e8c 100644 (file)
--- a/src/snmp_core.cc
+++ b/src/snmp_core.cc
@@ -781,7 +781,7 @@ client_Inst(oid * name, snint * len, mib_tree_entry * current, oid_ParseFn ** Fn
 {
     oid *instance = nullptr;
     Ip::Address laddr;
-    Ip::Address *aux;
+    const Ip::Address *aux;
     int size = 0;
     int newshift = 0;
 
@@ -807,9 +807,8 @@ client_Inst(oid * name, snint * len, mib_tree_entry * current, oid_ParseFn ** Fn
             *len += size ;
         }
     } else {
-        int shift = *len - current->len ; // i.e 4 or 16
-        oid2addr(&name[*len - shift], laddr,shift);
-        aux = client_entry(&laddr);
+        const auto ip = oid2addr(&name[current->len], *len - current->len);
+        aux = ip ? client_entry(&ip.value()) : nullptr;
         if (aux)
             laddr = *aux;
         else
@@ -1048,7 +1047,7 @@ snmpCreateOid(int length,...)
  * Debug calls, prints out the OID for debugging purposes.
  */
 const char *
-snmpDebugOid(oid * Name, snint Len, MemBuf &outbuf)
+snmpDebugOid(const oid * const Name, const snint Len, MemBuf &outbuf)
 {
     char mbuf[16];
     int x;
@@ -1105,26 +1104,30 @@ addr2oid(Ip::Address &addr, oid * Dest)
    oid == 32.1.50.239.162.33.251.20.50.0.0.0.0.0.0.0.0.0.1 ==>
    IPv6 address : 20:01:32:ef:a2:21:fb:32:00:00:00:00:00:00:00:00:OO:01
 */
-void
-oid2addr(oid * id, Ip::Address &addr, u_int size)
+std::optional<Ip::Address>
+oid2addr(const oid * const id, const size_t size)
 {
+    MemBuf tmp;
+    debugs(49, 7, "oid=" << snmpDebugOid(id, size, tmp));
+
     struct in_addr i4addr;
     struct in6_addr i6addr;
     u_int i;
     u_char *cp;
     if ( size == sizeof(struct in_addr) )
         cp = (u_char *) &(i4addr.s_addr);
-    else
+    else if ( size == sizeof(struct in6_addr) )
         cp = (u_char *) &(i6addr);
-    MemBuf tmp;
-    debugs(49, 7, "oid2addr: id : " << snmpDebugOid(id, size, tmp) );
+    else
+        return std::nullopt;
+
     for (i=0 ; i<size; ++i) {
         cp[i] = id[i];
     }
     if ( size == sizeof(struct in_addr) )
-        addr = i4addr;
+        return i4addr;
     else
-        addr = i6addr;
+        return i6addr;
 }
 
 int

diff --git a/src/snmp_core.h b/src/snmp_core.h
index 5583d9a97083b0fca5319929ccff3bc21eb0566d..29b5a25788d74f04dd603364ab4af75800b225de 100644 (file)
--- a/src/snmp_core.h
+++ b/src/snmp_core.h
@@ -18,6 +18,8 @@
 #include "ip/forward.h"
 #include "snmp_vars.h"
 
+#include <optional>
+
 class MemBuf;
 
 #define SNMP_REQUEST_SIZE 4096
@@ -47,9 +49,12 @@ AggrType snmpAggrType(oid* Current, snint CurrentLen);
 extern Comm::ConnectionPointer snmpOutgoingConn;
 
 extern PF snmpHandleUdp;
-const char * snmpDebugOid(oid * Name, snint Len, MemBuf &outbuf);
+const char * snmpDebugOid(const oid *, snint Len, MemBuf &outbuf);
 void addr2oid(Ip::Address &addr, oid *Dest);
-void oid2addr(oid *Dest, Ip::Address &addr, u_int code);
+
+/// Parses raw OID suffix of a given size as an IP address.
+/// \returns nil if the suffix does not represent an IPv4 or IPv6 address
+std::optional<Ip::Address> oid2addr(const oid *, size_t);
 
 namespace Acl
 {

diff --git a/src/tests/stub_client_db.cc b/src/tests/stub_client_db.cc
index f49a85c762f4ad036b5db0e89d5bb54ebd152c29..df521d76853086e1d25ad8f6708e6bc85a978978 100644 (file)
--- a/src/tests/stub_client_db.cc
+++ b/src/tests/stub_client_db.cc
@@ -21,7 +21,7 @@ void clientdbSetWriteLimiter(ClientInfo *, const int,const double,const double)
 ClientInfo *clientdbGetInfo(const Ip::Address &) STUB_RETVAL(nullptr)
 #endif
 #if SQUID_SNMP
-Ip::Address *client_entry(Ip::Address *) STUB_RETVAL(nullptr)
+const Ip::Address *client_entry(const Ip::Address *) STUB_RETVAL(nullptr)
 variable_list *snmp_meshCtblFn(variable_list *, snint *) STUB_RETVAL(nullptr)
 #endif