Replace bundled snmplib with shared libnetsnmp by yadij · Pull Request #2454 · squid-cache/squid

yadij · 2026-06-29T11:50:38Z

The bundled library is a copy of the long deceased UCD-SNMP
implementation which has since been merged into the current
Net-SNMP implementation widely available as libnetsnmp.

Switch to using Net-SNMP as a shared library and remove this
outdated library code from Squid.

Net-SNMP is itself quite outdated in a lot of ways but, is
still more up to date than this old Squid code, and there
does not appear to be any better SNMP library available.

The bundled library is a copy of the long deceased UCD-SNMP implementation which has since been merged into the current NET-SNMP implementation widely available as libnetsnmp. Switch to using NET-SNMP as a shared library and remove this outdated library code from Squid.

yadij · 2026-06-29T11:57:08Z

Draft for now as it needs more testing, particularly of SNMP packet behaviours with multi-process Squid.

Squid is expected to build with --enable-snmp fine, and work as before with no visible changes.

yadij · 2026-06-29T12:01:15Z


    reset();
+
+// XXX: fix this copying for net-snmp struct members:


Not sure how many of these fields are relevant to Squids SNMP v1 / v2c implementation.

rousskov

PR description: The bundled library is a copy of the long deceased UCD-SNMP implementation which has since been merged into the current Net-SNMP implementation widely available as libnetsnmp.

Current Squid SNMP code is no longer a copy. We have fixed many problems in previously copied code. This difference is important when deciding how to handle SNMP problems in Squid. Please replace "is" with "was" (at least).

PR description: Switch to using Net-SNMP as a shared library ... Net-SNMP is itself quite outdated in a lot of ways but, is still more up to date than this old Squid code, and there does not appear to be any better SNMP library available.

I support migration to Net-SNMP, even though we would not be able to fix that library when new SNMP vulnerability reports against Squid start to come in. Judging by Net-SNMP activity on GitHub, there will be such reports. Those reports would be against code external to Squid. While that fact does not really help anybody who uses SNMP in Squid, cares about CVEs, but cannot invest in fixing Net-SNMP library, Squid Project does not have the resources to secure its SNMP code. We can only hope that we are going to waste less time on working around Net-SNMP bugs than on maintaining currently embedded SNMP code.

rousskov · 2026-06-29T14:09:08Z

        Must(community != nullptr);
        xfree(community);
    }
-    xfree(peername);


This PR contains a lot of unnecessary out-of-scope code changes. It is too difficult for me to untangle them from changes actually required to support the new/replacement library. Please remove all unnecessary changes from this PR, including changes that move otherwise-unchanged code around.

The code has gone through two levels of review before submission. These changes are the result of the "new code matches Squid coding style requirements" review.

The code has gone through two levels of review before submission. These changes are the result of the "new code matches Squid coding style requirements" review.

Since the end result is problematic, the reviewers made a mistake and/or their suggestions were misinterpreted/misapplied. Can you point me to the reviews you are using to reject my change request?

rousskov · 2026-06-29T14:12:20Z

 {
    switch (type) {
-    case SMI_INTEGER:
+    case ASN_INTEGER:


Can Squid define an SMI_INTEGER and similar constants instead of changing this and similar Squid code using those constants? If the answer is "yes", then we should do that, at least for the library migration PR.

That would be the bad practice of adding unnecessary technical debt.

The goal of this PR is to remove the old library, not to preserve its dead API forever.

That would be the bad practice of adding unnecessary technical debt.

We need to balance many evils here. Creating temporary debt may reduce the number of errors and speed up this PR acceptance. If those goals are valuable, then creating that debt can be the lesser evil.

The goal of this PR is to remove the old library, not to preserve its dead API forever.

A "forever" preservation has not been requested. The actual duration of that debt existence depends on many factors. We even have the power to limit it by this PR lifetime (although that may require higher cooperation levels than we were able to achieve in the last few years).

rousskov · 2026-06-29T14:28:33Z

        peername = xstrdup(session.peername);
    }
+    if (session.localname) {
+        localname = xstrdup(session.localname);


A different way is needed to correctly support copying of net-snmp objects. A migration PR should not implement that and should not add copying of members that Squid is not already using (or, where absolutely necessary, their direct equivalents).

For example, why do we need to copy localname in this PR?

The members of the session struct have changed between the SNMPv1 based library and the SNMPv3 based library. All the protocol-agnostic members need to be copied to preserve session consistency when the SNMPv1/2 is acting.

Only the SNMPv3 and AgentX protocol fields are possibly optional - since those are supposed to be rejected, or at least undefined behaviour from Squid perspective.

All the protocol-agnostic members need to be copied to preserve session consistency when the SNMPv1/2 is acting

How do you define "session consistency" in this context? Why is singling out localname among many other new fields necessary and sufficient?

I hope that this PR can leave localname and numerous other new snmp_session fields untouched/unmentioned (including packing/unpacking code) while other PR(s) can eventually focus on correctly identifying information that needs to be relayed and correctly relaying that information across Squid kid processes. If some library code is not compatible with that plan, let's explicitly disclose that incompatibility and discuss how to address it.

rousskov · 2026-06-30T12:46:54Z

 {
    switch (type) {
-    case SMI_INTEGER:
+    case ASN_INTEGER:


That would be the bad practice of adding unnecessary technical debt.

We need to balance many evils here. Creating temporary debt may reduce the number of errors and speed up this PR acceptance. If those goals are valuable, then creating that debt can be the lesser evil.

The goal of this PR is to remove the old library, not to preserve its dead API forever.

A "forever" preservation has not been requested. The actual duration of that debt existence depends on many factors. We even have the power to limit it by this PR lifetime (although that may require higher cooperation levels than we were able to achieve in the last few years).

rousskov · 2026-06-30T12:52:20Z

        Must(community != nullptr);
        xfree(community);
    }
-    xfree(peername);


The code has gone through two levels of review before submission. These changes are the result of the "new code matches Squid coding style requirements" review.

Since the end result is problematic, the reviewers made a mistake and/or their suggestions were misinterpreted/misapplied. Can you point me to the reviews you are using to reject my change request?

rousskov · 2026-06-30T13:20:14Z

        peername = xstrdup(session.peername);
    }
+    if (session.localname) {
+        localname = xstrdup(session.localname);


All the protocol-agnostic members need to be copied to preserve session consistency when the SNMPv1/2 is acting

How do you define "session consistency" in this context? Why is singling out localname among many other new fields necessary and sufficient?

I hope that this PR can leave localname and numerous other new snmp_session fields untouched/unmentioned (including packing/unpacking code) while other PR(s) can eventually focus on correctly identifying information that needs to be relayed and correctly relaying that information across Squid kid processes. If some library code is not compatible with that plan, let's explicitly disclose that incompatibility and discuss how to address it.

rousskov · 2026-06-30T13:35:10Z

+#define INETADDRESSTYPE_ENUMS
+
+#define INETADDRESSTYPE_UNKNOWN  0
+#define INETADDRESSTYPE_IPV4  1


This code is located inside #if HAVE_NET_SNMP_TYPES_H, implying that it is needed for compatibility with that external library header. AFAICT, modern net-snmp/types.h does not define these INETADDRESSTYPE_ constants. If the external library does not actually require us to define these constants, then please place these definitions into snmp_core.h (and drop the ones Squid is not using).

rousskov · 2026-06-30T13:46:38Z

 #include "ip/Address.h"
 #include "log/access_log.h"
 #include "mgr/Registration.h"
+#include "snmp_core.h"


AFAICT, snmp_core.h has no #if SQUID_SNMP protection. Why do we want to start including it unconditionally?

rousskov · 2026-06-30T13:56:10Z

I adjusted PR title to emphasize that we are not just removing a bundled library, but are replacing it with a shared library.

yadij added the S-waiting-for-QA QA team action is needed (and usually required) label Jun 29, 2026

yadij commented Jun 29, 2026

View reviewed changes

rousskov self-requested a review June 29, 2026 12:57

rousskov requested changes Jun 29, 2026

View reviewed changes

rousskov requested changes Jun 30, 2026

View reviewed changes

rousskov changed the title ~~Remove snmplib bundled with Squid~~ Replace snmplib bundled with Squid with libnetsnmp Jun 30, 2026

rousskov changed the title ~~Replace snmplib bundled with Squid with libnetsnmp~~ Replace bundled snmplib with shared libnetsnmp Jun 30, 2026


		reset();

		// XXX: fix this copying for net-snmp struct members:

Uh oh!

Conversation

yadij commented Jun 29, 2026

Uh oh!

yadij commented Jun 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov commented Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants