FS.h - File class goes boom - when virtual functions not overwritten

[KurtE]

@Paul (and) others...

I have been hit by this before and with working on head start to maybe extend the File/FS classes in follow on releases.

If anyone currently creates their own implementation of the File class and they do not override all of the virtual functions,
All sketches that use this File class will build just fine, however in the majority of cases, this will lead to fault if one of these
functions is called. Example bogus sketch:

#include <FS.h>
class boomFile : public File {
 public: 
  boomFile(const char *name) : name_(name) {};
  private:
  const char *name_;
};

void setup() {
  while (!Serial) ;
  Serial.begin(115200); 
  Serial.print(CrashReport);
  Serial.println("\n" __FILE__ " " __DATE__ " " __TIME__);
  boomFile bfile("GoBoom");
  delay(250);
  Serial.println("direct file no boom"); Serial.flush();
  while(Serial.read() == -1) ; 
  while(Serial.read() != -1) ; 
  Serial.println(bfile.position(), DEC); Serial.flush();

  Serial.println("wrapped file - press any key to go BOOM"); Serial.flush();
  while (Serial.read() == -1) ;
  File wrap(&bfile);

  Serial.println(wrap.position(), DEC);
  delay(250);
  Serial.println("Don't expect this line to print");
}

void loop() {
  
}

This sketch builds fine. And as the code suggests it will go boom:

CrashReport:
  A problem occurred at (system time) 8:54:10
  Code was executing from address 0xB3C
  CFSR: 82
	(DACCVIOL) Data Access Violation
	(MMARVALID) Accessed Address: 0x1 (nullptr)
	  Check code at 0xB3C - very likely a bug!
	  Run "addr2line -e mysketch.ino.elf 0xB3C" for filename & line number.
  Temperature inside the chip was 49.43 °C
  Startup CPU clock speed is 600MHz
  Reboot was caused by auto reboot after fault or bad interrupt detected

C:\Users\kurte\Documents\Arduino\abc\abc.ino Aug 28 2021 08:54:00
direct file no boom

The problem: All of these methods are implemented like:

	virtual uint64_t position() {
		return (f) ? f->position() : 0;
	}

Now if I call it directly: (f) will be nullptr and as such it returns 0...
However in the wrapped case( File wrap(&bfile); ) f is not null, so it calls through null pointer.

Note: the wrapping is normal here... As for example from LittleFS you see things like:

					return File(new LittleFSFile(&lfs, dir, filepath));

Now where it is hitting me is suppose you wish to extend the File class, example:

 	virtual bool getAccessDateTime(uint16_t* pdate, uint16_t* ptime) {
  		return (f) ? f->getAccessDateTime(pdate, ptime) : false;
  	}

Without changing any additional code all of the current File Classes will build without issue, but calling these new methods will blow up this way...

So question is how to avoid going BOOM? Yes hopefully will have all new implementations for our known File classes, BUT if we miss any than...

Actually I see the issue:

private:
	void invalidate() {
		if (f && --(f->refcount) == 0) delete f;
	}
	union {
		// instances of base File class use this pointer
		File *f;
		// instances of derived classes (which actually access media)
		// use this reference count which is managed by the base class
		unsigned int refcount;
	};

So when I call through from the wrapper class where f points to in my case: boomFile class, which you now incremented the count to 1, which is in the same memory location as f...
1 is not NULLPTR so you try call through it...

So probably get rid of union?

[KurtE]

I see two simple solutions:

a) Remove the Union - Which I have done (commented out union line and } line, plus had to initialize refCount in constructor...
Most obvious solution...

b) Hackarama solution: saves a little memory:
Change all from like:

 	virtual bool getAccessDateTime(uint16_t* pdate, uint16_t* ptime) {
  		return (f) ? f->getAccessDateTime(pdate, ptime) : false;
  	}

to something like:

 	virtual bool getAccessDateTime(uint16_t* pdate, uint16_t* ptime) {
  		return (f & 0xffff) ? f->getAccessDateTime(pdate, ptime) : false;
  	}

Figuring probably don't need to worry about more than 65535 usage count and no memory pointers will be MSB=0...

Preference and/or alternatives?

Kurt

[Frank B]

Hackarama solutions are never good, longterm.

[KurtE]

Hackarama solutions are never good, longterm.

Thanks,

I hear you, which is why so far I have been working with option1 (removed the union)...
And pushed up those changes to my FS_Integration branch... So far only have one T4s will do T3.x ones real soon, as will be trying out the MTP code there as well

[PaulStoffregen]

If anyone currently creates their own implementation of the File class and they do not override all of the virtual functions,

I didn't really consider that case. My intention was derived classes must override all virtual functions.

Preference and/or alternatives?

Sorry, I don't have a good answer right now. But I do now see the problem (sorry, didn't see this thread yesterday). Really need to think about this for a while...

[WMXZ]

what is bad about requiring the implementation of all virtual functions?

[Frank B]

Sometimes there is no correct way to do that.
Imagine write() on a read only system. Sure you can always return 0 or false - but that is confusing for the user, as Paul said in an other thread. The docs require a write().
Edit: And even open() has a flag for the filemode.... what do you do if FILE_WRITE is set on a R/O FS?

This all boils down to not having assert(). Or a at least a replacement for assert(). We need a way to report coding errors.

The core is full of examples where it just ignores faulty calls. An assert() would help here, too.
Try to use a lot of DMA channels.... it silently does nothing when no channel is left. That's confusing. Or just a bug..int pin = 815; digitalWrite(pin, HIGH) -> No error report.
You can find a hundred examples more.
Yes, that's the Arduino way to handle it. That does not mean that it is good.

Edit: Errormessage -> no confused user, no mystic behaviour of the code, countless hours less of debugging. (NO i don't mention the dedicated error led.. no..no.... ;-)

[luni]

My intention was derived classes must override all virtual functions.

I'm probably missing something here but, if derived classes must override all virtual functions, couldn't you simply define them as pure virtual in the base class?
I.e.:

class myBase
{
    virtual void test() = 0; 
};

// won't compile if this class is used and the method 'test()' is not defined...
class derived : public myBase  
{
     //void test() override { /*do something*/}   
};

[PaulStoffregen]

couldn't you simply define them as pure virtual in the base class?

Yes, this is probably the correct solution, or at least an essential part of a good solution.

I tried to do it all with just 1 class. My usual approach could be called "less is more". I wanted to use less code and only 1 class to read & understand. But in hindsight, that now looks like a poor choice. It should probably be 3 classes, abstract base, referencing (the pointer part of the union), and implementation (the ref count part of the union). Maybe?

[luni]

"less is more"

Here we say "buy cheap and you'll buy twice" ;-)

Anyway, sounds like an interesting problem. For the fun of it I can try to work out an alternative solution for discussion. Any (non obvious) constraints?

[PaulStoffregen]

The main issue is File is trying to be 2 different things. One usage is a base class for actual library which implements access to storage media. The other use is just a reference to those.

I see the ESP folks are using 2 classes named File and FileImpl. I wanted to make it simple with just 1 class, but in hindsight that probably wasn't a great idea.

A secondary issue is how to do reference counting.

They're also using std::shared_ptr, which makes for very clean looking source code, but I suspect the result is another layer of allocations on the heap. Reality is I'm not familiar with exactly how newlib is implementing std::shared_ptr.

[Frank B]

Most likely.
It probably wants working mutexes, too.

https://en.cppreference.com/w/cpp/memory/shared_ptr

[PaulStoffregen]

Yeah, this stuff...

The control block is a dynamically-allocated object that holds:
- either a pointer to the managed object or the managed object itself;
- the deleter (type-erased);
- the allocator (type-erased);
- the number of shared_ptrs that own the managed object;
- the number of weak_ptrs that refer to the managed object.

It sure makes the source code look pretty, but underneath it's (probably) allocating this control block on the heap. Maybe that's ok and not too much? But it sure seems like a lot compared to putting a simple ref count into the implementation class and some less beautiful code into the referencing class.

[KurtE]

Morning all: background, where we ran into this recently:

Trying to extend File object to support dates/time. Personally it is nice to be able to see the dates and times especially when integrated with things like MTP and MSC.
The library SDFat does currently support this. Earlier it was only used/available in the directory listing, but now it is exposed at the sdfat file level. So trying to expose this..

So added: to the file object...

#ifdef FS_FILE_SUPPORT_DATES
	// These will all return false as only some FS support it.
  	virtual bool getAccessDateTime(uint16_t* pdate, uint16_t* ptime) {
  		return (f) ? f->getAccessDateTime(pdate, ptime) : false;
  	}
  	virtual bool getCreateDateTime(uint16_t* pdate, uint16_t* ptime) {
  		return (f) ? f->getCreateDateTime(pdate, ptime) : false;
  	}
  	virtual bool getModifyDateTime(uint16_t* pdate, uint16_t* ptime) {
  		return (f) ? f->getModifyDateTime(pdate, ptime) : false;
  	}
  	virtual bool timestamp(uint8_t flags, uint16_t year, uint8_t month, uint8_t day,
                 uint8_t hour, uint8_t minute, uint8_t second) {
  		return (f) ? f->timestamp(flags, year, month, day, hour, minute, second) : false;
  	}
#endif

And as I mentioned have versions of libraries to implement this, at least at the SDFat, MSC, MTP. With LittleFS so far have it just returning false. However LittleFS does have hooks and write ups on how others have implemented this, at least for Create and Modify dates, which will look at if/when we can start integrating this...

So ran into the MTP sketch faulting when I tried to view some directory as I forgot to pull in one of the updated libraries...

I can see at least three options on how to resolve the crash and/or avoid the issue:

a) Solutions I mentioned. Remove the union so (f) is either nullptr or valid pointer. Or Hack of assuming that usage count will fit into N bits lets say 16 and that pointers will never have a value of 0 for the higher bits...

b) Pure virtual base class - At build time it will fail to build if the sub-class has not implemented all of the functions. Good for catching it... Maybe a pain for extending.

c) Have the base class methods all return some error value like above: virtual bool getAccessDateTime(uint16_t* pdate, uint16_t* ptime) {return false;}

You could obviously do a combination of b) and c)

But my guess is that with b) and c) you would then need to probably update all of the libraries like LittleFS that does things like:

	File open(const char *filepath, uint8_t mode = FILE_READ) {
		//Serial.println("LittleFS open");
		if (!mounted) return File();
		if (mode == FILE_READ) {
			struct lfs_info info;
			if (lfs_stat(&lfs, filepath, &info) < 0) return File();
			//Serial.printf("LittleFS open got info, name=%s\n", info.name);
			if (info.type == LFS_TYPE_REG) {
				//Serial.println("  regular file");
				lfs_file_t *file = (lfs_file_t *)malloc(sizeof(lfs_file_t));
				if (!file) return File();
				if (lfs_file_open(&lfs, file, filepath, LFS_O_RDONLY) >= 0) {
					return File(new LittleFSFile(&lfs, file, filepath));
				}
				free(file);

Where you need to change the File(new... with something like: UsageCounterFile(...

[Frank B]

Not sure if it helps here or not..
in the waveplayer I just use a static global variable as counter (I just need the # of the current instance)

It increments on create.

But if you ned different ones, it might be needed to split everything to different files (compilation units). I don't know the details of "File".

[luni]

I played a bit with the code from FS.h and agree with Paul that separating the File class in an abstract interface (class FileAPI) and the File class is a good idea. The 'new' file class then only handles the relaying of calls to the derived classes. The actual File classes are required to implement the FileAPI (i.e., derive from the abstract FileAPI class). I declared this class as 'final' to prevent accidental deriving from it instead from FileAPI.

• Regarding the shared_pointer: I don't think a shared_pointer makes sense here. AFAIK, they are meant to be used for distributing pointers to various consumers. In this case the pointer never leaves the class, so, nothing to share...
• Union: I didn't understand the reason for the union? Is this only meant to save one uint32 or did I miss something?
• Regarding your comment in the File constructor:
  

  File(File *file) {
  		// "file" must only be a class derived from File
  		// can we use is_same or is_polymorphic with static_assert?
  		// or is_base_of
  		//static_assert(std::is_same<decltype(*file),File>::value,
  			//"File(File *file) constructor only accepts pointers "
  			//"to derived classes, not File itself");
  		f = file;
  		if (f) f->refcount++;
  	}

  The spitting into the FileAPI and the relay class fixes this automatically

Here a prove of principle implementation (only a few methods done): https://github.com/luni64/FileTest/b...er/src/tstFS.h For testing I used and adapted Franks MemFile class: https://github.com/luni64/FileTest/b...c/tstMemFile.h

The user interface did not change of course:

#include "Arduino.h"
#include "tstMemFile.h"


MemFS memFS;

char buf[1000];

void setup()
{
    while (!Serial) {}

    File f = memFS.open(buf, 1000, FILE_WRITE);
    char txt[] = "Hallo";
    f.write(txt, strlen(txt));

    Serial.println(buf);
}

void loop()
{
}

I could finalize this and test it with the various file systems, but only if it is of interest. If not, it was interesting to dive into this anyway :-)

[PaulStoffregen]

a) Solutions I mentioned. Remove the union so (f) is either nullptr or valid pointer. Or Hack of assuming that usage count will fit into N bits lets say 16 and that pointers will never have a value of 0 for the higher bits...

Sorry, I should have commented more directly on these earlier. While these ways do solve the immediate crashing problem, they do so by allowing usage in wrong ways. I know it seems to make things work, but in the long term this is going to cause problems.

b) Pure virtual base class - At build time it will fail to build if the sub-class has not implemented all of the functions. Good for catching it... Maybe a pain for extending.

Yes.

[PaulStoffregen]

Union: I didn't understand the reason for the union? Is this only meant to save one uint32 or did I miss something?

The union is about more than just saving 4 bytes.

The current File class is trying to be 2 different things. The implementation instances always use the reference count variable. The referencing instances always use the pointer.

[luni]

b) Pure virtual base class - At build time it will fail to build if the sub-class has not implemented all of the functions. Good for catching it... Maybe a pain for extending.
Yes.

Not necessarily. As you wrote elsewhere the API of the File class is pretty well defined and users expect a 'File' to have exactly this interface. So I'd make the methods required by the interface pure virtual to make sure they get implemented. Later extensions to the API can be normal virtual functions with default implementations. So, existing implementations wouldn't be affected by the extension. But of course this is a matter of taste.

[Frank B]

This is the difference between theory and practice.
Why should there be a write() in a r/o filesystem? An open with a mode flag?
Why the directory handling, when there are no directories? (i.e. spiffs)

Having it always the same interface is extremely unflexible. there is no common minimum of api calls that would work in every case.
The only common thing is the file pointer/refcount.

[luni]

Because then you can write code which just takes a 'File' and does not depend on the actual implementation of this file?
Here a funny to read stackoverflow answer on this topic: https://stackoverflow.com/a/384067/1842762

[Frank B]

As we've seen yesterday with MemFile, some think that is not compatible, because of the documentation (which says a file is always on SD, by the way) So.. your argument got killed yesterday
Plus, it needs a different open() and has no filenames. Despite of that, everything works, but....

No, this is no new discussion / argumentation pro Memfile. It's just an example. I don't want to discuss Memfile any further. I just use it ;-)

If someday a new FS comes, that needs a different open with a madatory extension, "Then you have the salad" as we Germans say.

[PaulStoffregen]

Why should there be a write() in a r/o filesystem? An open with a mode flag?

Major breaking changes to Arduino File & SD library API is not open for debate. Or at least not here, regarding File & FS.

But extensions like timestamps and event callbacks are likely in the future.

Why the directory handling, when there are no directories? (i.e. spiffs)

Having it always the same interface is extremely unflexible. there is no common minimum of api calls that would work in every case.

File & FS are exactly that, a common minimum set of function calls for accessing files stored on a filesystem. The answer to "why" is because that's what an API is.

Yes, I know you personally don't like it based on your experience yesterday trying to create a minimal non-filesystem library mapped onto the File API. When you abruptly closed that thread, it appeared this conversation had ended. But now you're restarting it here.

I want to say this as nicely & politely as possible: the File & FS APIs are meant for files stored on filesystems and asking to make it something else isn't open for debate. The API is mostly set in stone by over a decade of stability of the Arduino SD library. If you want a different type of API that's lighter and meant for abstract data not organized as files on a filesystem, maybe such an API could be created and maybe people would use it. But changes to the well established File & FS / SD library API to change its purpose is off topic.

[KurtE]

Sorry, I should have commented more directly on these earlier. While these ways do solve the immediate crashing problem, they do so by allowing usage in wrong ways. I know it seems to make things work, but in the long term this is going to cause problems.

Yep - I guess the main question will be just how much of a change are you going to want to make and how important will it be to maintain compatibility.
Example if you go to two classes or a pure virtual base class, then places who implement the FS's will need to update their code for the new class layout.

I started off with the assumption that we will need to maintain compatibility.

The union is about more than just saving 4 bytes.

The current File class is trying to be 2 different things. The implementation instances always use the reference count variable. The referencing instances always use the pointer.

Again a minimal change sort of hack solution could be something like:

define a #define or inline method like:
inline bool referencingObject() {return (f & 0xffff0000); }
(or could be >> 16 or ...

then:

	virtual size_t read(void *buf, size_t nbyte) {
		return (f) ? f->read(buf, nbyte) : 0;
	}

becomes:

	virtual size_t read(void *buf, size_t nbyte) {
		return referencingObject() ? f->read(buf, nbyte) : 0;
	}

Again a kludge, which was part of my a) except I show it as a method instead of doing it inline...

Will wait to see what is decided on...

I mainly also brought this up now as on the off chance you wish to put some changes in for the next release. Yes I know the following release is the main release to enhance some of this stuff

Kurt

[Frank B]

No, I'm not restarting the discussion. I just took it as example.