[SIM800 + AVR] Using String::endsWith() to compare RAM and Flash strings is incorrect

[Jurek822]

In waitResponse() there is using String::endsWith(const String &s2 ). Every time as an argument const __FlashStringHelper* object is used, and it is comparing with a string from RAM. However, in AVR endsWith() uses strcmp() to compare strings. This is a problem because for comparing const char * placed in RAM with const char * placed in FLASH strcmp_P() should be used. Alternatively, a const __FlashStringHelper* string may be cast to RAM string, for example:

    GsmConstStr r1=GFP(GSM_OK);
    String r1a;
    if (r1) r1a = r1;  // operator= uses strlen_P() and strcpy_P()
    // ... 
    if (r1 && data.endsWith(r1a)) {

[vshymanskyy]

Does it cause issues for you? It looks like if this is really a bug, it would just never work (but it currently works on AVR)

[Jurek822]

Yes, it is an issue for me. It is especially noticed in Diagnostic.ino example, because in this example results of waitResponse() are checked several times, for example:

if (!modem.restart()) {
   SerialMon.println(" fail");

But in BlynkClient.ino the result of modem.restart() is not checked and in case of unrecognized response a timeout will work. Moreover, I think that #60 is caused by this issue.

[vshymanskyy]

@Jurek822 , let me invite you to our chat: https://gitter.im/tinygsm/Developers

[vshymanskyy]

@Jurek822 any ideas how to fix this?

[Jurek822]

I see two ways:

1. Creating an own method which do the same as endsWith() but comparing RAM string with Flash String (without using RAM).
2. Converting Flash string into RAM string inside waitResponse() and using existing endsWith().

Ad. 1. The function may look like:

    unsigned char endsWith2(const String & ramStr, GsmConstStr flashStr) {
        if (!flashStr)
            return 0;
        const char * flashStr2 = (PGM_P) flashStr;
        int flashLen = strlen_P(flashStr2);
        int ramLen = ramStr.length();
        if (ramLen < flashLen)
            return 0;
        String substring = ramStr.substring(ramLen - flashLen);
        return strncmp_P(substring.c_str(), flashStr2, flashLen) == 0;
    }

Ad. 2. Look at the code in my first post.

[vshymanskyy]

@Jurek822, it should be very easy to validate that this is really causing your problems. Just switch it off with #if 0: https://github.com/vshymanskyy/TinyGSM/blob/master/TinyGsmCommon.h#L32

[Jurek822]

I have done some deep analysis. It turned out that: 1) GSM_OK from 24 line of TinyGsmClientSIM800.h is located at address 1068 (probably Flash); 2) during execution of waitResponse() variable data has address 2248 and r1 equals to GSM_OK has address 2216 - both probably in RAM; 3) the address of r1=GSM_OK is not the same for every call of waitResponse() Conclusion: during calling waitRespponse() variable r1 is converted from Flash string into RAM string.

[vshymanskyy]

So at least it is not a bug - just something we can improve on.

[Jurek822]

Yes, it is not a bug.