Seek mode "SeekEnd" attempts to read outside of file

[Aircoookie]

First off all, thank you so much @lorol for making the best embedded FS easily usable on ESP32! It's highly appreciated :)

I've ran into a problem with the seek() function, specifically in SeekEnd mode. It doesn't set the position correctly and attempting to read just causes 255 to be read. On closer inspection it seems like SeekEnd takes the file end and adds the offset to it (instead of subtracting), thus setting the position outsite the file. I would have tried setting a negative offset, but the seek() function only takes an unsigned integer.

Should it matter, this is with CONFIG_LITTLEFS_FOR_IDF_3_2 defined to enable support for the current ESP32 core release 1.0.4.

MCVE

#include <Arduino.h>
#include "LITTLEFS.h"

void printChar(char r) {
  Serial.print(r);
  Serial.print(" (");
  Serial.print((byte)r);
  Serial.println(")");
}

void setup() {
  Serial.begin(112500);
  delay(2000);

  LITTLEFS.begin(true);
  File f = LITTLEFS.open("/test.txt", "w");
  f.print("test1234");
  f.close();
  Serial.println("Wrote file.");

  f = LITTLEFS.open("/test.txt", "r");
  uint32_t fsize = f.size();

  //this is very inefficient, for demo purposes only
  Serial.println("Reading with SeekSet");
  Serial.print("File size ");
  Serial.println(f.size());
  for (int i = 0; i < fsize; i++) {
    f.seek(i,SeekSet);
    Serial.print("pos ");
    Serial.print(f.position());
    Serial.print(" --> ");
    
    printChar(f.read());
  }

  Serial.println("Reading with SeekEnd");
  for (int i = 1; i < fsize + 1; i++) {
    f.seek(i,SeekEnd);
    Serial.print("pos ");
    Serial.print(f.position());
    Serial.print(" --> ");

    printChar(f.read());
  }

}

void loop() {

}

Output

Wrote file.
Reading with SeekSet
File size 8
pos 0 --> t (116)
pos 1 --> e (101)
pos 2 --> s (115)
pos 3 --> t (116)
pos 4 --> 1 (49)
pos 5 --> 2 (50)
pos 6 --> 3 (51)
pos 7 --> 4 (52)
Reading with SeekEnd
pos 9 --> � (255)
pos 10 --> � (255)
pos 11 --> � (255)
pos 12 --> � (255)
pos 13 --> � (255)
pos 14 --> � (255)
pos 15 --> � (255)
pos 16 --> � (255)

For reference, the correct output from the ESP8266 version of LittleFS

Wrote file.
Reading with SeekSet
File size 8
pos 0 --> t (116)
pos 1 --> e (101)   
pos 2 --> s (115)   
pos 3 --> t (116)   
pos 4 --> 1 (49)    
pos 5 --> 2 (50)
pos 6 --> 3 (51)
pos 7 --> 4 (52)
Reading with SeekEnd
pos 7 --> 4 (52)
pos 6 --> 3 (51)
pos 5 --> 2 (50)
pos 4 --> 1 (49)
pos 3 --> t (116)
pos 2 --> s (115)
pos 1 --> e (101)
pos 0 --> t (116)

[Aircoookie]

Of course this can be easiely worked around using f.seek(f.size() - offset), but I still would have expected SeekEnd to function correctly :)

[lorol]

esp8266/Arduino#7323
https://github.com/esp8266/Arduino/pull/7324/files
For reference.
Will check when I have more time.
We may need @BrianPugh intervention again :)

[Aircoookie]

Thank you :) 👍

For the PR you linked, this part from line 376 of LittleFS.h is interesting:

int32_t offset = static_cast<int32_t>(pos);
if (mode == SeekEnd) {
  offset = -offset; // TODO - this seems like its plain wrong vs. POSIX
}

That seems to explain what the 8266 guys have done differently - that comment indicates that they were also in doubt how to implement it properly.
It is true that it is quite unintuitive to have a positive offset and then subtracting it - but then again I can't think of any real use case for seeking past the end of file. If anything, the seek() function should take a signed integer (which would also make a lot of sense for the SeekCur mode). But on ESP8266 that probably clashes with the existing wrapper from SPIFFS and I don't know if it would be a good fit for your wrapper either as it is technically a breaking change.

[lorol]

Yes, I see same thing.

        case SEEK_END: whence = LFS_SEEK_END; offset = -offset; break;

This change of esp_littlefs.c seems to fix it, I just checked on your example
I prefer @BrianPugh to apply it first at original esp_littlefs side. But you can test it.

[BrianPugh]

i'll implement it soon, just double checking thats absolutely what we want to do. Looking at upstream littlefs code, it seems like we do want to make the offset negative, just double checking that everything is "expected behavior"

[lorol]

Sure, no rush :)

[BrianPugh]

a lot of online documentation for *seek commands indicate:

The new position, measured in bytes, is obtained by adding offset bytes to the position specified by whence.

so it seems like the current behavior is expected and the 8266 implementation is wrong? And the comment on their code indicates that they know it // TODO - this seems like its plain wrong vs. POSIX. So I'm assuming they were doing it to be backwards compatible with something else that implemented it wrong (maybe SPIFFS?). I can add another compile-time macro to make it compatible if we desire; super simple change.

Sources:
https://man7.org/linux/man-pages/man3/fseek.3.html

[lorol]

Hm ..
Maybe within same define, we already we used for SPIFFS compat?
And leave the pain of choice when @me-no-dev builds-in next IDF files to Arduino core :)
Here is easy, just one define to comment or no.

[lorol]

No ...
Guess what. on esp32 SPIFFS implementation is exactly as it is now in LITTLEFS , not inverted. and results are the same as given here at beginning,

Opps ... not same but similar, just stuck to 8 ?!

So maybe the esp8266 should not be followed ...

E (2007) SPIFFS: mount failed, -10025
(formating .....)
Wrote file.
Reading with SeekSet
File size 8
pos 0 --> t (116)
pos 1 --> e (101)
pos 2 --> s (115)
pos 3 --> t (116)
pos 4 --> 1 (49)
pos 5 --> 2 (50)
pos 6 --> 3 (51)
pos 7 --> 4 (52)
Reading with SeekEnd
pos 8 --> ⸮ (255)
pos 8 --> ⸮ (255)
pos 8 --> ⸮ (255)
pos 8 --> ⸮ (255)
pos 8 --> ⸮ (255)
pos 8 --> ⸮ (255)
pos 8 --> ⸮ (255)
pos 8 --> ⸮ (255)

[Aircoookie]

I agree that a define sounds like the most sensible solution 👍
Seems like the "wrong" behavior dates back all the way to the OG Arduino seek() function meant for SD cards. That implementation had no whence attribute and thus it made sense to use an unsigned long type for the offset. For whence other than SEEK_SET to make sense though, offset needs to be signed. Seems like ESP8266 SPIFFS or some other project kept that unsigned for compatibility but instead chose to not be POSIX-compliant.

Interesting find @lorol !! I wouldn't have expected SPIFFS on ESP32 to behave the same, in that case I would suggest keeping the current behavior as the default as ESP32 users upgrading from SPIFFS should rather expect 100% identical behavior than ESP8266 LittleFS users.

[BrianPugh]

@lorol maybe this macro should be in your layer rather than mine, since mine "doesn't know" about arduino

[lorol]

@BrianPugh is it allowed pos to point beyond the file end?

[BrianPugh]

It does not. In the event that you attempt to index past the size of the file, it will return LFS_ERR_INVAL. The file position will remain as it was before the call.

https://github.com/littlefs-project/littlefs/blob/4c9146ea539f72749d6cc3ea076372a81b12cb11/lfs.c#L3025

[lorol]

So the example above should not go to pos 9, 10 ... but it does?
It is basically https://github.com/espressif/arduino-esp32/blob/master/libraries/FS/src/vfs_api.cpp#L352

[BrianPugh]

actually I was wrong, that FILE_MAX is the maximum file size supported by the FS. Seeking beyond the file end is allowed.

[lorol]

In this case it is correct to return true and see position after the end.
Which means at esp32 SPIFFS is different. it gives the file end position and returns false

[lorol]

I think, we should leave as it is. We cannot and shouldn't always run after SPIFFS and/or esp8266.
As maximum, the above comment behavior may go into CONFIG_LITTLEFS_SPIFFS_COMPAT and that's it.

[BrianPugh]

according to arduino documentation, if you seek beyond the size of the file it:

1. Returns false
2. Doesn't explicitly define what should happen to the file pos when false is returned.

https://www.arduino.cc/en/Reference/FileSeek

[lorol]

Just searched the SPIFFS ... way
https://github.com/pellepl/spiffs/blob/master/src/spiffs_hydrogen.c#L597

:) I wanted to keep everything directly from your implementation. For things like this maybe code writing is required ...
Not sure actually how to make it, but I like to learn the things.

[lorol]

I tested with FFat on esp32. Same results as LITTLEFS.
No changes. I am leaving as it is and I am closing the case.
@Aircoookie, there is workaround shown above. Use it if you cannot find another way like pass a negative pos.
The Arduino file.seek() does not specify the mode, so with default mode unsigned long is OK.
Unfortunately, on FFat and now on LITTLEFS, it will return true even after the end of file boundary (up to max file size of the system). so you have to do a workaround at application level.
Use for example file.size() to limit to the actual file size.