More correct elf loader (#1839)

* ELF File: more robust section loader
* ELF File: faster and smaller preinit, init and fini arrays handling
* ELF File: load sections on preload stage
* ELF File: naming
* Furi: fix use after free in thread join

Co-authored-by: あく <alleteam@gmail.com>

applications/main/fap_loader/fap_loader_app.c

@@ -70,6 +70,7 @@ static bool fap_loader_run_selected_app(FapLoader* loader) {
     do {
         file_selected = true;
         loader->app = flipper_application_alloc(loader->storage, &hashtable_api_interface);
+        size_t start = furi_get_tick();
 
         FURI_LOG_I(TAG, "FAP Loader is loading %s", furi_string_get_cstr(loader->fap_path));
 
@@ -99,6 +100,7 @@ static bool fap_loader_run_selected_app(FapLoader* loader) {
             break;
         }
 
+        FURI_LOG_I(TAG, "Loaded in %ums", (size_t)(furi_get_tick() - start));
         FURI_LOG_I(TAG, "FAP Loader is staring app");
 
         FuriThread* thread = flipper_application_spawn(loader->app, NULL);

furi/core/thread.c

@@ -103,6 +103,7 @@ static void furi_thread_body(void* context) {
     furi_assert(pvTaskGetThreadLocalStoragePointer(NULL, 0) != NULL);
     vTaskSetThreadLocalStoragePointer(NULL, 0, NULL);
 
+    thread->task_handle = NULL;
     vTaskDelete(NULL);
     furi_thread_catch();
 }
@@ -211,13 +212,8 @@ bool furi_thread_join(FuriThread* thread) {
 
     furi_check(furi_thread_get_current() != thread);
 
-    // Check if thread was started
-    if(thread->task_handle == NULL) {
-        return false;
-    }
-
     // Wait for thread to stop
-    while(eTaskGetState(thread->task_handle) != eDeleted) {
+    while(thread->task_handle) {
         furi_delay_ms(10);
     }
 

lib/flipper_application/elf/elf_file.c

@@ -57,8 +57,23 @@ static ELFSection* elf_file_get_section(ELFFile* elf, const char* name) {
     return ELFSectionDict_get(elf->sections, name);
 }
 
-static void elf_file_put_section(ELFFile* elf, const char* name, ELFSection* section) {
-    ELFSectionDict_set_at(elf->sections, strdup(name), *section);
+static ELFSection* elf_file_get_or_put_section(ELFFile* elf, const char* name) {
+    ELFSection* section_p = elf_file_get_section(elf, name);
+    if(!section_p) {
+        ELFSectionDict_set_at(
+            elf->sections,
+            strdup(name),
+            (ELFSection){
+                .data = NULL,
+                .sec_idx = 0,
+                .size = 0,
+                .rel_count = 0,
+                .rel_offset = 0,
+            });
+        section_p = elf_file_get_section(elf, name);
+    }
+
+    return section_p;
 }
 
 static bool elf_read_string_from_offset(ELFFile* elf, off_t offset, FuriString* name) {
@@ -320,12 +335,12 @@ static bool elf_relocate_symbol(ELFFile* elf, Elf32_Addr relAddr, int type, Elf3
     return true;
 }
 
-static bool elf_relocate(ELFFile* elf, Elf32_Shdr* h, ELFSection* s) {
+static bool elf_relocate(ELFFile* elf, ELFSection* s) {
     if(s->data) {
         Elf32_Rel rel;
-        size_t relEntries = h->sh_size / sizeof(rel);
+        size_t relEntries = s->rel_count;
         size_t relCount;
-        (void)storage_file_seek(elf->fd, h->sh_offset, true);
+        (void)storage_file_seek(elf->fd, s->rel_offset, true);
         FURI_LOG_D(TAG, " Offset   Info     Type             Name");
 
         int relocate_result = true;
@@ -395,14 +410,6 @@ static bool elf_relocate(ELFFile* elf, Elf32_Shdr* h, ELFSection* s) {
     return false;
 }
 
-/**************************************************************************************************/
-/********************************************* MISC ***********************************************/
-/**************************************************************************************************/
-
-static bool cstr_prefix(const char* prefix, const char* string) {
-    return strncmp(prefix, string, strlen(prefix)) == 0;
-}
-
 /**************************************************************************************************/
 /************************************ Internal FAP interfaces *************************************/
 /**************************************************************************************************/
@@ -445,6 +452,31 @@ static bool elf_load_debug_link(ELFFile* elf, Elf32_Shdr* section_header) {
                section_header->sh_size;
 }
 
+static bool elf_load_section_data(ELFFile* elf, ELFSection* section, Elf32_Shdr* section_header) {
+    if(section_header->sh_size == 0) {
+        FURI_LOG_D(TAG, "No data for section");
+        return true;
+    }
+
+    section->data = aligned_malloc(section_header->sh_size, section_header->sh_addralign);
+    section->size = section_header->sh_size;
+
+    if(section_header->sh_type == SHT_NOBITS) {
+        // BSS section, no data to load
+        return true;
+    }
+
+    if((!storage_file_seek(elf->fd, section_header->sh_offset, true)) ||
+       (storage_file_read(elf->fd, section->data, section_header->sh_size) !=
+        section_header->sh_size)) {
+        FURI_LOG_E(TAG, "    seek/read fail");
+        return false;
+    }
+
+    FURI_LOG_D(TAG, "0x%X", section->data);
+    return true;
+}
+
 static SectionType elf_preload_section(
     ELFFile* elf,
     size_t section_idx,
@@ -453,73 +485,63 @@ static SectionType elf_preload_section(
     FlipperApplicationManifest* manifest) {
     const char* name = furi_string_get_cstr(name_string);
 
-    const struct {
-        const char* prefix;
-        SectionType type;
-    } lookup_sections[] = {
-        {".text", SectionTypeData},
-        {".rodata", SectionTypeData},
-        {".data", SectionTypeData},
-        {".bss", SectionTypeData},
-        {".preinit_array", SectionTypeData},
-        {".init_array", SectionTypeData},
-        {".fini_array", SectionTypeData},
-        {".rel.text", SectionTypeRelData},
-        {".rel.rodata", SectionTypeRelData},
-        {".rel.data", SectionTypeRelData},
-        {".rel.preinit_array", SectionTypeRelData},
-        {".rel.init_array", SectionTypeRelData},
-        {".rel.fini_array", SectionTypeRelData},
-    };
-
-    for(size_t i = 0; i < COUNT_OF(lookup_sections); i++) {
-        if(cstr_prefix(lookup_sections[i].prefix, name)) {
-            FURI_LOG_D(TAG, "Found section %s", lookup_sections[i].prefix);
-
-            if(lookup_sections[i].type == SectionTypeRelData) {
-                name = name + strlen(".rel");
-            }
-
-            ELFSection* section_p = elf_file_get_section(elf, name);
-            if(!section_p) {
-                ELFSection section = {
-                    .data = NULL,
-                    .sec_idx = 0,
-                    .rel_sec_idx = 0,
-                    .size = 0,
-                };
-
-                elf_file_put_section(elf, name, &section);
-                section_p = elf_file_get_section(elf, name);
-            }
-
-            if(lookup_sections[i].type == SectionTypeRelData) {
-                section_p->rel_sec_idx = section_idx;
-            } else {
-                section_p->sec_idx = section_idx;
-            }
+    // Load allocable section
+    if(section_header->sh_flags & SHF_ALLOC) {
+        ELFSection* section_p = elf_file_get_or_put_section(elf, name);
+        section_p->sec_idx = section_idx;
+
+        if(section_header->sh_type == SHT_PREINIT_ARRAY) {
+            elf->preinit_array = section_p;
+        } else if(section_header->sh_type == SHT_INIT_ARRAY) {
+            elf->init_array = section_p;
+        } else if(section_header->sh_type == SHT_FINI_ARRAY) {
+            elf->fini_array = section_p;
+        }
 
-            return lookup_sections[i].type;
+        if(!elf_load_section_data(elf, section_p, section_header)) {
+            FURI_LOG_E(TAG, "Error loading section '%s'", name);
+            return SectionTypeERROR;
+        } else {
+            return SectionTypeData;
         }
     }
 
+    // Load link info section
+    if(section_header->sh_flags & SHF_INFO_LINK) {
+        name = name + strlen(".rel");
+        ELFSection* section_p = elf_file_get_or_put_section(elf, name);
+        section_p->rel_count = section_header->sh_size / sizeof(Elf32_Rel);
+        section_p->rel_offset = section_header->sh_offset;
+        return SectionTypeRelData;
+    }
+
+    // Load symbol table
     if(strcmp(name, ".symtab") == 0) {
         FURI_LOG_D(TAG, "Found .symtab section");
         elf->symbol_table = section_header->sh_offset;
         elf->symbol_count = section_header->sh_size / sizeof(Elf32_Sym);
         return SectionTypeSymTab;
-    } else if(strcmp(name, ".strtab") == 0) {
+    }
+
+    // Load string table
+    if(strcmp(name, ".strtab") == 0) {
         FURI_LOG_D(TAG, "Found .strtab section");
         elf->symbol_table_strings = section_header->sh_offset;
         return SectionTypeStrTab;
-    } else if(strcmp(name, ".fapmeta") == 0) {
+    }
+
+    // Load manifest section
+    if(strcmp(name, ".fapmeta") == 0) {
         FURI_LOG_D(TAG, "Found .fapmeta section");
         if(elf_load_metadata(elf, section_header, manifest)) {
             return SectionTypeManifest;
         } else {
             return SectionTypeERROR;
         }
-    } else if(strcmp(name, ".gnu_debuglink") == 0) {
+    }
+
+    // Load debug link section
+    if(strcmp(name, ".gnu_debuglink") == 0) {
         FURI_LOG_D(TAG, "Found .gnu_debuglink section");
         if(elf_load_debug_link(elf, section_header)) {
             return SectionTypeDebugLink;
@@ -531,61 +553,17 @@ static SectionType elf_preload_section(
     return SectionTypeUnused;
 }
 
-static bool elf_load_section_data(ELFFile* elf, ELFSection* section) {
-    Elf32_Shdr section_header;
-    if(section->sec_idx == 0) {
-        FURI_LOG_D(TAG, "Section is not present");
-        return true;
-    }
-
-    if(!elf_read_section_header(elf, section->sec_idx, &section_header)) {
-        return false;
-    }
-
-    if(section_header.sh_size == 0) {
-        FURI_LOG_D(TAG, "No data for section");
-        return true;
-    }
-
-    section->data = aligned_malloc(section_header.sh_size, section_header.sh_addralign);
-    section->size = section_header.sh_size;
-
-    if(section_header.sh_type == SHT_NOBITS) {
-        /* section is empty (.bss?) */
-        /* no need to memset - allocator already did that */
-        return true;
-    }
-
-    if((!storage_file_seek(elf->fd, section_header.sh_offset, true)) ||
-       (storage_file_read(elf->fd, section->data, section_header.sh_size) !=
-        section_header.sh_size)) {
-        FURI_LOG_E(TAG, "    seek/read fail");
-        return false;
-    }
-
-    FURI_LOG_D(TAG, "0x%X", section->data);
-    return true;
-}
-
 static bool elf_relocate_section(ELFFile* elf, ELFSection* section) {
-    Elf32_Shdr section_header;
-    if(section->rel_sec_idx) {
+    if(section->rel_count) {
         FURI_LOG_D(TAG, "Relocating section");
-        if(elf_read_section_header(elf, section->rel_sec_idx, &section_header))
-            return elf_relocate(elf, &section_header, section);
-        else {
-            FURI_LOG_E(TAG, "Error reading section header");
-            return false;
-        }
+        return elf_relocate(elf, section);
     } else {
         FURI_LOG_D(TAG, "No relocation index"); /* Not an error */
     }
     return true;
 }
 
-static void elf_file_call_section_list(ELFFile* elf, const char* name, bool reverse_order) {
-    ELFSection* section = elf_file_get_section(elf, name);
-
+static void elf_file_call_section_list(ELFSection* section, bool reverse_order) {
     if(section && section->size) {
         const uint32_t* start = section->data;
         const uint32_t* end = section->data + section->size;
@@ -729,7 +707,6 @@ bool elf_file_load_section_table(ELFFile* elf, FlipperApplicationManifest* manif
     }
 
     furi_string_free(name);
-    FURI_LOG_D(TAG, "Load symbols done");
 
     return IS_FLAGS_SET(loaded_sections, SectionTypeValid);
 }
@@ -739,16 +716,6 @@ ELFFileLoadStatus elf_file_load_sections(ELFFile* elf) {
     ELFSectionDict_it_t it;
 
     AddressCache_init(elf->relocation_cache);
-    size_t start = furi_get_tick();
-
-    for(ELFSectionDict_it(it, elf->sections); !ELFSectionDict_end_p(it); ELFSectionDict_next(it)) {
-        ELFSectionDict_itref_t* itref = ELFSectionDict_ref(it);
-        FURI_LOG_D(TAG, "Loading section '%s'", itref->key);
-        if(!elf_load_section_data(elf, &itref->value)) {
-            FURI_LOG_E(TAG, "Error loading section '%s'", itref->key);
-            status = ELFFileLoadStatusUnspecifiedError;
-        }
-    }
 
     if(status == ELFFileLoadStatusSuccess) {
         for(ELFSectionDict_it(it, elf->sections); !ELFSectionDict_end_p(it);
@@ -777,14 +744,13 @@ ELFFileLoadStatus elf_file_load_sections(ELFFile* elf) {
     FURI_LOG_D(TAG, "Relocation cache size: %u", AddressCache_size(elf->relocation_cache));
     FURI_LOG_D(TAG, "Trampoline cache size: %u", AddressCache_size(elf->trampoline_cache));
     AddressCache_clear(elf->relocation_cache);
-    FURI_LOG_I(TAG, "Loaded in %ums", (size_t)(furi_get_tick() - start));
 
     return status;
 }
 
 void elf_file_pre_run(ELFFile* elf) {
-    elf_file_call_section_list(elf, ".preinit_array", false);
-    elf_file_call_section_list(elf, ".init_array", false);
+    elf_file_call_section_list(elf->preinit_array, false);
+    elf_file_call_section_list(elf->init_array, false);
 }
 
 int32_t elf_file_run(ELFFile* elf, void* args) {
@@ -794,7 +760,7 @@ int32_t elf_file_run(ELFFile* elf, void* args) {
 }
 
 void elf_file_post_run(ELFFile* elf) {
-    elf_file_call_section_list(elf, ".fini_array", true);
+    elf_file_call_section_list(elf->fini_array, true);
 }
 
 const ElfApiInterface* elf_file_get_api_interface(ELFFile* elf_file) {

lib/flipper_application/elf/elf_file_i.h

@@ -16,8 +16,10 @@ typedef int32_t(entry_t)(void*);
 typedef struct {
     void* data;
     uint16_t sec_idx;
-    uint16_t rel_sec_idx;
     Elf32_Word size;
+
+    size_t rel_count;
+    Elf32_Off rel_offset;
 } ELFSection;
 
 DICT_DEF2(ELFSectionDict, const char*, M_CSTR_OPLIST, ELFSection, M_POD_OPLIST)
@@ -39,6 +41,10 @@ struct ELFFile {
     File* fd;
     const ElfApiInterface* api_interface;
     ELFDebugLinkInfo debug_link_info;
+
+    ELFSection* preinit_array;
+    ELFSection* init_array;
+    ELFSection* fini_array;
 };
 
 #ifdef __cplusplus