work buffer passed to SPIFFS_mount must be aligned on some platforms

[girtsf]

I'm seeing SPIFFS_mount() crash due to what looks like an unaligned memory access. Specifically, trying to access obj_lu_buf[cur_entry-entry_offset]. This is on an ARM Cortex-M0. I do have SPIFFS_ALIGNED_OBJECT_INDEX_TABLES set to 1.

Looks like SPIFFS_mount() fixes fd_space and cache alignment, but work and lu_work does not get fixed or checked. Perhaps SPIFFS_ALIGNED_OBJECT_INDEX_TABLES could be extended to something like SPIFFS_REQUIRE_ALIGNED_ACCESS, which would also verify that the pointers passed in align at 32b boundary. Though there might be architectures with stricter alignment requirements.

Also, I did have to specify -Wno-cast-align, otherwise I'm getting "cast increases required alignment of target type" warnings in a bunch of places, mostly related to fs->work and fs->lu_work usage. Let me know if you want me to file a separate bug for this.

(And thanks for the great work!)

[girtsf]

I haven't fully thought it through, but perhaps a way to handle this would be to define a struct for each of fs->work and fs->l_work members that has a bunch of unions for each type of access that the spiffs code might want to do, e.g.,

typedef struct {
  struct {
    union {
      // Access as bytes.
      u8_t* as_u8;
      //  Access page header + pages. 
      struct {
        spiffs_page_header header;
        spiffs_page_ix pages[0];
      } as_something;
      // As obj ids.
      struct {
        spiffs_obj_id obj_ids[0];
      } as_obj_ids;
    };
  } lu_work;
  struct {
    union {
      // Access as bytes.
      u8_t* as_u8;
      // <Add other access views here...>
    };
  } work;
} spiffs_work_buffer;

The caller can then instantiate spiffs_work_buffer (either statically, or on the heap with the right alignment), and the compiler should take care of the right alignment. In all the places where fs->work and fs->lu_work is cast to structures currently, it would be replaced with something like fs->work_buffer.lu_work.header, etc.

I'm probably missing something obvious on why this is a bad idea (other than slightly more verbosity).

[pellepl]

Hi,

In my opinion that is a good idea as it is more verbose than casting things everywhere, and I wished I would have done it that way from the start. It would take some effort rewriting it now as the work buffers are used more or less everywhere. I'll have to muster up some courage to do this :)

What compiler (and version) are you using? I could try to reproduce at least the warnings here. As for the actual crash, what if you align the work buffer itself passed to SPIFFS_mount - will that fix it? E.g. something like

u8_t my_spiffs_work_buffer[2*SPIFFS_LOG_PAGE_SIZE] __attribute__ ((aligned (sizeof(void *))));

[girtsf]

What compiler (and version) are you using?

$ arm-none-eabi-gcc --version arm-none-eabi-gcc (GNU Tools for ARM Embedded Processors) 5.4.1 20160919 (release) [ARM/embedded-5-branch revision 240496]

Basically, in any place that has a stored u8_t* that gets cast to some structure that has alignment requirements (e.g., a u32_t in it), this warning will show up.

If I align the work buffer, then things work fine. That's what I ended up doing.