Skip to content

Conversation

@N1IOX
Copy link
Contributor

@N1IOX N1IOX commented Jan 14, 2026

Unit tests were failing:

tests.cpp:68:19: error: ‘void* memcpy(void*, const void*, size_t)’ reading 10 bytes from a region of size 8 [-Werror=stringop-overread]
   68 |             memcpy(&buffer[i], &randomLong, 10);
      |             ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tests.cpp:67:18: note: source object ‘randomLong’ of size 8
   67 |             long randomLong = random();
      |                  ^~~~~~~~~~

Use sizeof(randomLong)

Note, though, that after fixing this, the unit test can still fail - warned as much in the code: "This may SOMETIMES fail if the random gets lucky and makes something valid"
At least on my machine, this seems to happen more than sometimes...this particular test might need revisiting.


Also address a compiler warning:

../src/BERDecode.cpp:92:25: warning: operation on ‘tempVal’ may be undefined [-Wsequence-point]
   92 |                 tempVal = tempVal |= 0xFF000000;
      |                 ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~

0neblock and others added 5 commits August 9, 2024 07:13
a length of exactly 256 should be encoded as:
0x82 0x01 0x00

without this change, I was seeing the encoded length of a get response (that happened to be exactly 256 bytes long) encoded as:
0x81 0x00

that was causing snmpbulkwalk to fail, since it was reading the encoded length as 0.
@N1IOX
Copy link
Contributor Author

N1IOX commented Jan 14, 2026

@0neblock

Per the comment in the code, this one specific unit test does fail, and not just sometimes:

    SECTION( "Should fail to parse a corrupt buffer "){
        SNMPPacket* readPacket = new SNMPPacket();
        for(int i = 25; i < 133; i+= 10){
            char old[10] = {0};
            memcpy(old, &buffer[i], 10);
            long randomLong = random();
            memcpy(&buffer[i], &randomLong, sizeof(randomLong));
            // This may SOMETIMES fail if the random gets lucky and makes something valid
            REQUIRE( readPacket->parseFrom(buffer, 200) != SNMP_ERROR_OK );

            memcpy(&buffer[i], old, 10);
            REQUIRE( readPacket->parseFrom(buffer, 200) == SNMP_ERROR_OK );
        }
    }

Both on the automated test machine used on GH, and on a machine I have here, it ends up failing on the 7th pass through that loop...succeeding 6 times before that 7th pass. Consistently.

Results output:

tests.cpp:70: PASSED:
  REQUIRE( readPacket->parseFrom(buffer, 200) != 1 )
with expansion:
  -13 != 1
...
tests.cpp:70: PASSED:
...
tests.cpp:70: PASSED:
...
tests.cpp:70: PASSED:
...
tests.cpp:70: PASSED:
...
tests.cpp:70: PASSED:
...
tests.cpp:70: FAILED:
  REQUIRE( readPacket->parseFrom(buffer, 200) != 1 )
with expansion:
  1 != 1

I was going to merely assume that the 8 bytes of "corruption" landed in the middle of one of the OIDs, whilst still leaving the packet parsable...but you know what they say about those who assume ;)
So instead of that, I added a quick hex dump routine to tests.cpp, and called it before and corruption.
Sure enough, that is the case. Hex dumps attached:

Original.txt
Corrupted.txt

The latter contains the hex dump from 7th pass. Both are "manually" parsed.

The diff of these ends up landing in the final 8 bytes used by test OID ".1.3.6.1.4.1.52420.9999999":

06 0C 2B 06 01 04 01 83 99 44 84 E2 AC 7F
06 0C 2B 06 01 04 F2 41 B1 2E 00 00 00 00

I suggest disabling this one test for now, and revisiting it later.
Agreed?

N1IOX added a commit to N1IOX/Arduino_SNMP that referenced this pull request Jan 15, 2026
…it test cases has an issue, resulting in false negatives. This then prevents the automated workflow from succeeding.

***Temporarily*** disable the one unit test: "Should fail to parse a corrupt buffer"
This test needs to be modified to ensure that it always results in the intended test outcome, perhaps by not relying on random data.
@N1IOX N1IOX mentioned this pull request Jan 15, 2026
0neblock added a commit that referenced this pull request Jan 15, 2026
* Fix some invalid decoded lengths from various

* length values >= 256 need 3 bytes to encode their length

a length of exactly 256 should be encoded as:
0x82 0x01 0x00

without this change, I was seeing the encoded length of a get response (that happened to be exactly 256 bytes long) encoded as:
0x81 0x00

that was causing snmpbulkwalk to fail, since it was reading the encoded length as 0.

* use sizeof

* fix a compiler warning treated as an error: "operation on ‘tempVal’ may be undefined"

* As documented in the comments of PR 59 ( #59 ), one of the unit test cases has an issue, resulting in false negatives.  This then prevents the automated workflow from succeeding.

***Temporarily*** disable the one unit test: "Should fail to parse a corrupt buffer"
This test needs to be modified to ensure that it always results in the intended test outcome, perhaps by not relying on random data.

---------

Co-authored-by: Aidan Cyr <aidan@aidancyr.dev>
@N1IOX
Copy link
Contributor Author

N1IOX commented Jan 15, 2026

Clean up: closing the PRs previously blocked by unit tests

@N1IOX N1IOX closed this Jan 15, 2026
@N1IOX N1IOX deleted the tests-fail-length-of-long branch January 15, 2026 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants