Memory Leak When Handling Requests or Responses Without Vectors

[gonzabrusco]

While reviewing your SNMP library, I discovered a significant memory leak related to how SNMP requests and responses are handled.

For example, when a response is added to a SNMPMessage a Varbind object is created:

SNMP/src/BER.h

Lines 1982 to 1990 in b5d3c73

	VarBind(const char *oid, BER *value = nullptr) :
	ArrayBER(Type::Sequence) {
	add(new ObjectIdentifierBER(oid));
	if (value) {
	add(value);
	} else {
	add(new NullBER);
	}
	}

That in part creates a ObjectIdentifierBER that it then passes to add():

SNMP/src/BER.h

Lines 1899 to 1911 in b5d3c73

	BER* add(BER *ber) {
	#if SNMP_VECTOR
	_bers.push_back(ber);
	_length += ber->getSize();
	_count++;
	#else
	if (_count < U) {
	_bers[_count++] = ber;
	_length += ber->getSize();
	}
	#endif
	return ber;
	}

If vectors are not enabled (#define SNMP_VECTOR 0) and the BER array reaches its limit, the add() function silently fails to store the ObjectIdentifierBER—but the memory allocated for it is never freed. This results in a memory leak.

Here's a memory usage graph showing DRAM usage on an ESP32 when making successive bulk SNMP requests that exceed the capacity of the BER array:

Enabling vector support (#define SNMP_VECTOR 1) prevents the leak, but it removes the ability to enforce a fixed limit on the request size—something that may be important in constrained environments.

Probably this changes will need a big refactor.

[patricklaf]

Hi @gonzabrusco ,
Thanks for the issue.

This is the result of offering support for low end board, i.e. Uno. In order to reduce code size, as stated in the readme in the limitations section, there is practically no check done.

May be it's better to use vector.

[patricklaf]

BTW, I closed this issue too quickly, by mistake.

Fill free to reopen it and continue the discussion.

[gonzabrusco]

Reopening it! Maybe the simplest way of doing is that the add() methods returns a nullptr if there's no more room to hold the new BER. If I have time, I will send you a PR.

PS: Apparently I do not have permission to reopen this issue.

[gonzabrusco]

Before releasing another version with the length fix, it would be nice to check this.

What I’d like to see addressed:

• No memory leaks when VECTORS are disabled: Currently, disabling VECTORS can lead to memory leaks when the ArrayBER gets bigger than template<const uint8_t U>.
• Limit the number of Varbinds when VECTORS are enabled: Allow setting a maximum size for the vector. Letting it grow indefinitely could be risky—especially in constrained environments. A malicious or malformed SNMP request could easily exhaust memory and crash the agent.

I will try to make a PR for this, but I can't promise anything.