MFRC522Extended.cpp:824: warning: ordered comparison of pointer with integer zero

[szotsaki]

The following GCC warning is emitted during compile:

MFRC522Extended.cpp:824: warning: ordered comparison of pointer with integer zero [-Wextra]
  if (backData && (backLen > 0)) {
                             ^

Since backLen is a pointer, it will be always positive. However, for fixing it I don't know if it's a bug

• not being backLen != nullptr or
• not being *backLen > 0 or
• just a slight oversight and the comparison is unnecessary.

The same is also in line 847.

[Rotzbua]

I agree that there is a unclear situation. But I have no overview about this code. Main coding is from JPG-Consulting but he finished his project.

Links to lines of code:
https://github.com/miguelbalboa/rfid/blame/master/src/MFRC522Extended.cpp#L824
https://github.com/miguelbalboa/rfid/blame/master/src/MFRC522Extended.cpp#L847

[JM-FRANCE]

My guess is that it is to check that the pointer is non NULL as you read from/write to the pointed address in case the if condition is true.

--> just change if (backData && (backLen > 0)) into if (backData && (backLen != nullptr))

(edit - disregard)

[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?

[JM-FRANCE]

backLen is a pointer, writing (backLen > 0) is not correct
It drives the warning "ordered comparison of pointer with integer zero"

[RalphCorderoy]

Hi @JAY-M-ERICSSON, Yes, that's the problem being fixed. You seem to be taking a quote of the current faulty code as a suggestion for the fix. :-)

[JM-FRANCE]

fair point indeed

[pktl]

Hi everyone,
I encountered this issue with a twist: What you described as a warning message is an error on my machine, preventing me from compiling at all.

/home/pb/Arduino/libraries/MFRC522/src/MFRC522Extended.cpp: In member function 'MFRC522::StatusCode MFRC522Extended::TCL_Transceive(MFRC522Extended::TagInfo*, byte*, byte, byte*, byte*)':
/home/pb/Arduino/libraries/MFRC522/src/MFRC522Extended.cpp:824:34: error: ordered comparison of pointer with integer zero ('byte*' {aka 'unsigned char*'} and 'int')
  824 |         if (backData && (backLen > 0)) {
      |                          ~~~~~~~~^~~
/home/pb/Arduino/libraries/MFRC522/src/MFRC522Extended.cpp:847:42: error: ordered comparison of pointer with integer zero ('byte*' {aka 'unsigned char*'} and 'int')
  847 |                 if (backData && (backLen > 0)) {
      |                                  ~~~~~~~~^~~

I'm using Arduino IDE 1.8.16 and MFRC522 v1.4.10, if that matters.
Are there any compiler flags that might be set differently on my machine?

This is what's executed on compile:

/usr/bin/avr-g++ -c -g -Os -Wall -Wextra -std=gnu++11 -fpermissive -fno-exceptions -ffunction-sections -fdata-sections -fno-threadsafe-statics -Wno-error=narrowing -MMD -flto -mmcu=atmega328p -DF_CPU=16000000L -DARDUINO=10816 -DARDUINO_AVR_NANO -DARDUINO_ARCH_AVR -I/usr/share/arduino/hardware/archlinux-arduino/avr/cores/arduino -I/usr/share/arduino/hardware/archlinux-arduino/avr/variants/eightanaloginputs -I/home/pb/Arduino/libraries/DFPlayer_Mini_Mp3_by_Makuna/src -I/usr/share/arduino/hardware/archlinux-arduino/avr/libraries/EEPROM/src -I/home/pb/Arduino/libraries/JC_Button/src -I/home/pb/Arduino/libraries/MFRC522/src -I/usr/share/arduino/hardware/archlinux-arduino/avr/libraries/SPI/src -I/usr/share/arduino/hardware/archlinux-arduino/avr/libraries/SoftwareSerial/src /home/pb/Arduino/libraries/MFRC522/src/MFRC522Extended.cpp -o /tmp/arduino_build_940207/libraries/MFRC522/MFRC522Extended.cpp.o

[JM-FRANCE]

Hi everyone, I encountered this issue with a twist: What you described as a warning message is an error on my machine, preventing me from compiling at all.

have you modified the library taking into account the suggestion from @RalphCorderoy?

#371 (comment)

[pktl]

Ah, my mistake. Didn't read it as careful as I should have. I got it to compile with that fix.
Thanks!

[FallenAngel666]

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?

Confirm that. Works fine after change. Also Line 909 the creator of the code already uses the proper comparison.