patricklaf / SNMP

Simple Network Management Protocol library for Arduino
MIT License
10 stars 1 forks source link

fix: Cannot parse ObjectIdentifier with empty value #1

Closed aiotter closed 2 years ago

aiotter commented 2 years ago

On SequenceBER::decode, there is a chance that new ObjectIdentifierBER is created with nullptr. This causes panic by reading over prohibited area while executing ObjectIdentifierBER.setValue. This PR fixes this problem.

aiotter commented 2 years ago

Oh I forgot to thank you for providing a very useful library. I need a SNMP manager on ESP32 which can send GETNEXTREQUEST message. I really appreciate your effort!

patricklaf commented 2 years ago

@aiotter , thank you for your interest in this project and for this PR number one!

First of all, good catch, this is a real bug. Never encounter myself, may be it is platform specific. Are you using STM32 or other core/MCU?

In this case, ObjectIdentifierBER created in SequenceBER::decode, the ObjectIdentifierBER constructor take care of setting _value[0] to zero, so I think that it is not necessary to redo it immediately in ObjectIdentifierBER::setValue.

It could be useful in case of direct call to ObjectIdentifierBER::setValue after object creation. But I wonder why one would call it with nullptr parameter. The way the library is designed, all the SNMP objects are created, used and deleted as soon as possible to reduce memory usage, so why reuse an ObjectIdentifierBER instance and set its value to null?

So, before merging, I propose you to simply your fix this way (with 4 spaces to indent code, please):

void setValue(const char *value) {
    // L, V
    if (value) {
        strncpy(_value, value, SIZE_OBJECTIDENTIFIER);
    }
    unsigned int index = 0;
    ...
}
aiotter commented 2 years ago

I did it ;-)

I'm using ESP32 based M5Stack board with Arduino platform via PlatformIO. The problem won't be caused if the uninitialised area just after nullptr is filled with zero. It is highly possible that this is undefined behaviour and thus depends on environments.

patricklaf commented 2 years ago

Thank you, merging!