Ordered comparison of pointer with integer zero

[salieff]

Step 0: Are you in the right place?

Yes

Step 1: Describe your environment

• OS version: Fedora release 31 (Thirty One) Linux 5.6.8-200.fc31.x86_64 SMP
• Arduino IDE version: 1.8.12
• MFRC522 Library version: 1.4.6
• Arduino device: ESP-32
• MFRC522 device: RFID RC-522

Step 2: Describe the problem

Mistypo in if expression

Affected file(s) or example(s):

• MFRC522Extended.cpp

Steps to reproduce:

1. Use library in your project
2. Compile your project
3. Read compiler warnings carefully

Observed Results:

• Pointer to length was compared to positive instead of value points to

Expected Results:

• Length to be compared

Relevant Code:

--- MFRC522Extended.cpp	2020-01-21 14:25:01.000000000 +0300
+++ MFRC522Extended.cpp_new	2020-05-07 04:31:20.875876104 +0300
@@ -821,7 +821,7 @@
 	// Swap block number on success
 	tag->blockNumber = !tag->blockNumber;
 
-	if (backData && (backLen > 0)) {
+	if (backData && backLen && (*backLen > 0)) {
 		if (*backLen < in.inf.size)
 			return STATUS_NO_ROOM;
 
@@ -844,7 +844,7 @@
 		if (result != STATUS_OK)
 			return result;
 
-		if (backData && (backLen > 0)) {
+		if (backData && backLen && (*backLen > 0)) {
 			if ((*backLen + ackDataSize) > totalBackLen)
 				return STATUS_NO_ROOM;

[Rotzbua]

duplicate of #371

The code is never used for basic usage of this library. To fix this right you have to dig into the rfc specification and program code (there are some more smelling code). Or let it be... :-/

[RalphCorderoy]

Please consider reopening this issue as it blocks compiling an Arduino sketch with -Werror and ignoring warnings is generally a bad smell because other warnings mingle in with the known ones and get missed.

Besides, I think looking at the code shows up a bunch of problems in this area which suggest it has no users and could just be deleted or #ifdef'd out until it's all fixed.

The prototype allows two optional parameters, both pointers, both defaulting to NULL.

StatusCode TCL_Transceive(TagInfo * tag, byte *sendData, byte sendLen,
    byte *backData = NULL, byte *backLen = NULL);

The function body always unconditionally dereferences backLen.

byte totalBackLen = *backLen;

I don't know C++, only C, but I assume this is just as invalid in C++?

That implies backLen is never allowed to be NULL and thus must always be given but I don't think that's the author's intent so it's probably a bug and the parameter is intended to be optional and so NULL.

Then we have the two uses which guard the memcpy(3), e.g.

if (backData && (backLen > 0)) {
        if (*backLen < in.inf.size)
                return STATUS_NO_ROOM;

        *backLen = in.inf.size;
        memcpy(backData, in.inf.data, in.inf.size);
}

Given backLen may be NULL the if-condition must guard against it. The test that the length is big enough is already in the body and correct so I think this just needs to change to

if (backData && backLen) {

Similarly for the second case:

if (backData && (backLen > 0)) {
        if ((*backLen + ackDataSize) > totalBackLen)
                return STATUS_NO_ROOM;

        memcpy(&(backData[*backLen]), ackData, ackDataSize);
        *backLen += ackDataSize;
}

We've already saved the original size of backData by copying *backLen into totalBackLen and have altered *backLen to reflect what we've used so far so the body's test takes all that into account and adjusts memcpy()'s destination. All that's left is to correct the guard as before. Changing the test to *backLen > 0 wouldn't be much use and wouldn't allow for in.inf.size to be 0 in the first memcpy.

@salieff, does this seem accurate to you given you've submitted a patch in the past?

[Rotzbua]

Please consider reopening this issue

@RalphCorderoy Why reopen? I mentioned that it is a duplicate of #371 . Please write there instead of splitting a problem into multiple issue reports.

[RalphCorderoy]

Hi @Rotzbua, Sorry, I thought I was on that earlier issue. I've repeated the comment there.