tedit-cosmo integration as default GUI for e9studio#6
Conversation
Initial GUI framework for e9studio with: - Platform detection (Windows/Linux/macOS/BSD) - Backend auto-detection (Win32/X11/Cocoa/TUI) - TUI fallback when GUI unavailable - Plugin system architecture - Panel system for dockable views - Configuration system (INI-based) Design inspired by teditor's extensibility philosophy: - Simple core with plugin extensions - Cross-platform via Cosmopolitan Libc - Graceful degradation to TUI mode Files added: - gui/TEDITOR_DESIGN.md - Architecture documentation - gui/e9studio_gui.h - Public API header - gui/e9studio_gui.c - Core implementation (stubs) Next steps: - Implement Win32/X11/Cocoa backends - Wire up to e9analysis for disasm/decompile views - Add E9Scanner integration
- Add tedit-cosmo inspired application core (e9studio_app.c) - Gap buffer implementation for efficient text editing - Editor state with cursor, selection, and history tracking - File operations (load/save) with language detection - Menu system with INI configuration - Add unified entry point (e9studio_gui_main.c) - Auto-detect GUI/TUI/CLI mode based on environment - Self-test diagnostics for validation - Command-line argument parsing with help/version - Add CLI platform backend (e9gui_cli.c) - Text-based interface for binary analysis commands - Build system integration with variable substitution - Placeholder hooks for analysis engine integration - Add e9studio_tedit.h integration header - Bridge between tedit-cosmo and e9studio analysis - Application state with editor management - Platform abstraction interface - Add WASM payload build support (Makefile.wasm) - Build WAT files using wasm-as from binaryen - Optimize WASM with wasm-opt - Example payload generation - Update Makefile.e9studio - Add gui/cli build targets - Add wasm/binaryen targets - Include GUI sources with proper flags Tested: make gui passes with native gcc, self-test passes all checks.
There was a problem hiding this comment.
Pull request overview
This PR integrates a tedit-cosmo inspired GUI framework as the default interface for e9studio, with automatic fallback to CLI/TUI modes based on the environment. The implementation adds approximately 3,000 lines of new code organized as a modular GUI framework with platform abstraction.
Changes:
- Adds a new GUI framework with automatic backend detection (Win32, X11, Cocoa, TUI fallback)
- Implements a CLI platform backend with text-based commands for binary analysis
- Integrates a WASM payload build system using Binaryen tools
- Provides unified entry point that auto-detects appropriate mode (GUI/TUI/CLI)
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile.e9studio | Adds build targets for GUI (make gui), CLI (make cli), and WASM payloads (make wasm) |
| src/e9studio/wasm/Makefile.wasm | Build system for WASM payloads using Binaryen's wasm-as and wasm-opt |
| src/e9studio/gui/e9studio_gui.h | GUI framework API with platform detection, window management, and plugin system |
| src/e9studio/gui/e9studio_gui.c | Core GUI framework implementation with backend detection and TUI fallback |
| src/e9studio/gui/e9studio_tedit.h | Integration header bridging tedit-cosmo with e9studio analysis engine |
| src/e9studio/gui/e9studio_app.c | Application core with gap buffer editor, file operations, and menu system |
| src/e9studio/gui/e9studio_gui_main.c | Unified entry point with mode auto-detection and self-test diagnostics |
| src/e9studio/gui/platform/e9gui_cli.c | CLI platform backend with text-based binary analysis commands |
| src/e9studio/gui/config/menuini.txt | Menu configuration in INI format for extensibility |
| src/e9studio/gui/TEDITOR_DESIGN.md | Design documentation explaining architecture and extensibility philosophy |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| printf("Undo not yet implemented.\n"); | ||
| } | ||
|
|
||
| void e9editor_redo(E9EditorState *ed) | ||
| { | ||
| if (!ed) return; | ||
| /* TODO: Implement redo with history */ | ||
| printf("Redo not yet implemented.\n"); |
There was a problem hiding this comment.
The printf statements in undo/redo functions should not be present in production code. These should either be removed, converted to proper logging, or only printed in debug mode.
| printf("Undo not yet implemented.\n"); | |
| } | |
| void e9editor_redo(E9EditorState *ed) | |
| { | |
| if (!ed) return; | |
| /* TODO: Implement redo with history */ | |
| printf("Redo not yet implemented.\n"); | |
| } | |
| void e9editor_redo(E9EditorState *ed) | |
| { | |
| if (!ed) return; | |
| /* TODO: Implement redo with history */ |
src/e9studio/gui/e9studio_app.c
Outdated
| app->editor_capacity = app->editor_capacity ? app->editor_capacity * 2 : 4; | ||
| app->editors = realloc(app->editors, | ||
| app->editor_capacity * sizeof(E9EditorState *)); |
There was a problem hiding this comment.
The realloc calls for growing the editors array should check for allocation failure. If realloc fails, the original pointer will be lost, causing a memory leak. Store the result in a temporary variable first and check for NULL before assigning.
| app->editor_capacity = app->editor_capacity ? app->editor_capacity * 2 : 4; | |
| app->editors = realloc(app->editors, | |
| app->editor_capacity * sizeof(E9EditorState *)); | |
| size_t new_capacity = app->editor_capacity ? app->editor_capacity * 2 : 4; | |
| E9EditorState **new_editors = realloc(app->editors, | |
| new_capacity * sizeof(E9EditorState *)); | |
| if (!new_editors) { | |
| /* Allocation failed; do not lose existing editors array */ | |
| e9editor_destroy(ed); | |
| return NULL; | |
| } | |
| app->editors = new_editors; | |
| app->editor_capacity = new_capacity; |
src/e9studio/gui/e9studio_app.c
Outdated
| if (menus->menu_count >= menus->menu_capacity) { | ||
| menus->menu_capacity = menus->menu_capacity ? menus->menu_capacity * 2 : 8; | ||
| menus->menus = realloc(menus->menus, | ||
| menus->menu_capacity * sizeof(E9GuiMenu)); | ||
| } | ||
| current_menu = &menus->menus[menus->menu_count++]; | ||
| memset(current_menu, 0, sizeof(E9GuiMenu)); | ||
| strncpy(current_menu->label, s, sizeof(current_menu->label) - 1); | ||
| } | ||
| } else if (current_menu) { | ||
| /* Menu item */ | ||
| char *comma = strchr(s, ','); | ||
| if (comma) { | ||
| *comma = '\0'; | ||
| char *label = e9util_str_trim(s); | ||
| char *command = e9util_str_trim(comma + 1); | ||
|
|
||
| /* Add item to menu */ | ||
| if (current_menu->item_count >= current_menu->item_capacity) { | ||
| current_menu->item_capacity = current_menu->item_capacity ? | ||
| current_menu->item_capacity * 2 : 16; | ||
| current_menu->items = realloc(current_menu->items, | ||
| current_menu->item_capacity * sizeof(E9GuiMenuItem)); | ||
| } | ||
| E9GuiMenuItem *item = ¤t_menu->items[current_menu->item_count++]; | ||
| memset(item, 0, sizeof(E9GuiMenuItem)); | ||
| strncpy(item->label, label, sizeof(item->label) - 1); | ||
| strncpy(item->command, command, sizeof(item->command) - 1); | ||
| item->enabled = true; | ||
| } else if (s[0] == '-') { | ||
| /* Separator */ | ||
| if (current_menu->item_count >= current_menu->item_capacity) { | ||
| current_menu->item_capacity = current_menu->item_capacity ? | ||
| current_menu->item_capacity * 2 : 16; | ||
| current_menu->items = realloc(current_menu->items, | ||
| current_menu->item_capacity * sizeof(E9GuiMenuItem)); |
There was a problem hiding this comment.
The realloc calls for growing menu arrays should check for allocation failure to prevent memory leaks and undefined behavior. Store the result in a temporary variable and check for NULL before assigning.
src/e9studio/gui/e9studio_gui.c
Outdated
| win->panel_capacity = win->panel_capacity ? win->panel_capacity * 2 : 8; | ||
| win->panels = realloc(win->panels, win->panel_capacity * sizeof(E9Panel *)); |
There was a problem hiding this comment.
The realloc call for growing the panels array should check for allocation failure. If realloc returns NULL, the original pointer will be lost, causing a memory leak. Store the result in a temporary variable first.
| win->panel_capacity = win->panel_capacity ? win->panel_capacity * 2 : 8; | |
| win->panels = realloc(win->panels, win->panel_capacity * sizeof(E9Panel *)); | |
| size_t new_capacity = win->panel_capacity ? win->panel_capacity * 2 : 8; | |
| E9Panel **new_panels = realloc(win->panels, new_capacity * sizeof(E9Panel *)); | |
| if (!new_panels) { | |
| /* Allocation failed; clean up and leave window state unchanged */ | |
| free(panel); | |
| return NULL; | |
| } | |
| win->panels = new_panels; | |
| win->panel_capacity = new_capacity; |
| char cmd[64] = {0}; | ||
| char arg[512] = {0}; | ||
|
|
||
| sscanf(line, "%63s %511[^\n]", cmd, arg); |
There was a problem hiding this comment.
The sscanf call is vulnerable to buffer overflow if the input line contains more than 63 or 511 characters respectively. Consider validating input length or using safer alternatives like snprintf with length checks.
| (void)(buf->gap_end - buf->gap_start); /* gap_size available if needed */ | ||
|
|
There was a problem hiding this comment.
This ternary expression is computing gap_size but the result is unused (marked with void cast). If gap_size is not needed, this computation should be removed. If it's needed for future bounds checking, the void cast should be removed and the variable should be used.
| (void)(buf->gap_end - buf->gap_start); /* gap_size available if needed */ |
src/e9studio/gui/e9studio_app.c
Outdated
| if (!buf) return NULL; | ||
|
|
||
| /* TODO: Extract selection from buffer */ |
There was a problem hiding this comment.
The allocation of buf should check for malloc failure before attempting to extract the selection. Also, the TODO comment indicates this is incomplete - the selection extraction from the gap buffer is not implemented.
| if (!buf) return NULL; | |
| /* TODO: Extract selection from buffer */ | |
| if (!buf) { | |
| if (len) *len = 0; | |
| return NULL; | |
| } | |
| /* Extract selection from underlying gap buffer */ | |
| E9Buffer *buffer = ed->buffer; | |
| if (!buffer || !buffer->data) { | |
| free(buf); | |
| if (len) *len = 0; | |
| return NULL; | |
| } | |
| size_t gap_size = buffer->gap_end - buffer->gap_start; | |
| for (size_t i = 0; i < sel_len; ++i) { | |
| size_t idx = start + i; /* logical index */ | |
| size_t phys_idx; | |
| if (idx < buffer->gap_start) { | |
| phys_idx = idx; | |
| } else { | |
| phys_idx = idx + gap_size; | |
| } | |
| buf[i] = buffer->data[phys_idx]; | |
| } | |
| buf[sel_len] = '\0'; |
src/e9studio/gui/e9studio_gui.c
Outdated
| panel->text_capacity = len * 2; | ||
| panel->text_content = realloc(panel->text_content, panel->text_capacity); | ||
| } | ||
| strcpy(panel->text_content, text); |
There was a problem hiding this comment.
Using strcpy with a realloc'd buffer could be unsafe if realloc fails. The previous text_content pointer would be lost. Additionally, strcpy should be replaced with strncpy or memcpy to prevent potential buffer overflows.
| panel->text_capacity = len * 2; | |
| panel->text_content = realloc(panel->text_content, panel->text_capacity); | |
| } | |
| strcpy(panel->text_content, text); | |
| size_t new_capacity = len * 2; | |
| char *new_content = realloc(panel->text_content, new_capacity); | |
| if (!new_content) { | |
| /* Allocation failed; keep existing text_content and capacity unchanged. */ | |
| return; | |
| } | |
| panel->text_content = new_content; | |
| panel->text_capacity = new_capacity; | |
| } | |
| memcpy(panel->text_content, text, len); |
Makefile.e9studio
Outdated
| # WASM Payload Build | ||
| ######################################################################### | ||
|
|
||
| BINARYEN_PATH ?= /home/user/binaryen |
There was a problem hiding this comment.
The hardcoded path /home/user/binaryen is not portable and will fail on most systems. This should either use a relative path, be documented as requiring user configuration, or be removed as a default value.
| BINARYEN_PATH ?= /home/user/binaryen | |
| BINARYEN_PATH ?= ./binaryen |
| if (pos < buf->gap_start) { | ||
| /* Move gap left */ | ||
| size_t move = buf->gap_start - pos; | ||
| memmove(buf->data + buf->gap_end - move, buf->data + pos, move); | ||
| buf->gap_start = pos; | ||
| buf->gap_end -= move; |
There was a problem hiding this comment.
When moving the gap left, the calculation buf->gap_end -= move is incorrect. It should be buf->gap_end = buf->gap_start + (buf->gap_end - buf->gap_start) to maintain the gap size. The current implementation will incorrectly shrink the gap.
Address code review feedback: Memory Safety: - Add null-check protection for all realloc calls in panel, editor, and menu array growth to prevent memory leaks on allocation failure - Store realloc results in temporary variables before assignment - Replace unsafe strcpy with memcpy/strncpy for bounded string copies - Add explicit null-termination after sscanf parsing Portability: - Replace hardcoded /home/user/binaryen path with dynamic detection that checks ../binaryen (sibling), /opt/binaryen, then falls back to system PATH - Update both Makefile.e9studio and Makefile.wasm for consistent binaryen path handling All tests pass after changes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void e9gui_window_set_title(E9Window *win, const char *title) | ||
| { | ||
| if (!win || !title) return; | ||
| strncpy(win->title, title, sizeof(win->title) - 1); |
There was a problem hiding this comment.
The window title is copied using strncpy without ensuring null-termination. If the title string is equal to or longer than sizeof(win->title), the resulting string in win->title will not be null-terminated. This should be followed by explicit null-termination: win->title[sizeof(win->title) - 1] = '\0';
| return NULL; | ||
| } | ||
|
|
||
| strncpy(win->title, config->title ? config->title : "E9Studio", sizeof(win->title) - 1); |
There was a problem hiding this comment.
The window title is copied using strncpy without ensuring null-termination. If the config->title string is equal to or longer than sizeof(win->title), the resulting string will not be null-terminated. This should be followed by explicit null-termination: win->title[sizeof(win->title) - 1] = '\0';
| strncpy(win->title, config->title ? config->title : "E9Studio", sizeof(win->title) - 1); | |
| strncpy(win->title, config->title ? config->title : "E9Studio", sizeof(win->title) - 1); | |
| win->title[sizeof(win->title) - 1] = '\0'; |
src/e9studio/gui/e9studio_app.c
Outdated
| static void buffer_ensure_gap(E9Buffer *buf, size_t required) | ||
| { | ||
| size_t gap_size = buf->gap_end - buf->gap_start; | ||
| if (gap_size >= required) return; | ||
|
|
||
| /* Resize buffer */ | ||
| size_t new_capacity = buf->capacity + required + GAP_SIZE; | ||
| char *new_data = malloc(new_capacity); | ||
| if (!new_data) return; | ||
|
|
||
| /* Copy data before gap */ | ||
| memcpy(new_data, buf->data, buf->gap_start); | ||
|
|
||
| /* Copy data after gap to new location */ | ||
| size_t after_gap = buf->capacity - buf->gap_end; | ||
| memcpy(new_data + new_capacity - after_gap, buf->data + buf->gap_end, after_gap); | ||
|
|
||
| free(buf->data); | ||
| buf->data = new_data; | ||
| buf->gap_end = new_capacity - after_gap; | ||
| buf->capacity = new_capacity; | ||
| } |
There was a problem hiding this comment.
The buffer_ensure_gap function silently fails when memory allocation fails. This can lead to unexpected behavior when subsequent operations assume the gap was successfully resized. The function should either return an error code or set an error flag that can be checked by the caller.
| char *buf = malloc(sel_len + 1); | ||
| if (!buf) return NULL; | ||
|
|
||
| /* TODO: Extract selection from buffer */ | ||
| if (len) *len = sel_len; | ||
| return buf; |
There was a problem hiding this comment.
The e9editor_get_selection function allocates memory for the selection buffer but never populates it with actual data (line 327 indicates this is a TODO). This results in returning uninitialized memory to the caller, which is undefined behavior. The function should either be marked as unimplemented or properly extract the selection from the buffer.
| char *buf = malloc(sel_len + 1); | |
| if (!buf) return NULL; | |
| /* TODO: Extract selection from buffer */ | |
| if (len) *len = sel_len; | |
| return buf; | |
| /* TODO: Extract selection from buffer. Currently unimplemented. */ | |
| (void)start; | |
| (void)end; | |
| (void)sel_len; | |
| if (len) *len = 0; | |
| return NULL; |
| e9editor_set_text(ed, data, read); | ||
| free(data); | ||
|
|
||
| strncpy(ed->file_path, path, sizeof(ed->file_path) - 1); |
There was a problem hiding this comment.
The file path is copied using strncpy without ensuring null-termination. If the path string is equal to or longer than sizeof(ed->file_path), the resulting string in ed->file_path will not be null-terminated. This should be followed by an explicit null-termination: ed->file_path[sizeof(ed->file_path) - 1] = '\0';
| fclose(f); | ||
| free(data); | ||
|
|
||
| strncpy(ed->file_path, path, sizeof(ed->file_path) - 1); |
There was a problem hiding this comment.
The file path is copied using strncpy without ensuring null-termination. If the path string is equal to or longer than sizeof(ed->file_path), the resulting string in ed->file_path will not be null-terminated. This should be followed by an explicit null-termination: ed->file_path[sizeof(ed->file_path) - 1] = '\0';
| strncpy(current_menu->label, s, sizeof(current_menu->label) - 1); | ||
| } | ||
| } else if (current_menu) { | ||
| /* Menu item */ | ||
| char *comma = strchr(s, ','); | ||
| if (comma) { | ||
| *comma = '\0'; | ||
| char *label = e9util_str_trim(s); | ||
| char *command = e9util_str_trim(comma + 1); | ||
|
|
||
| /* Add item to menu */ | ||
| if (current_menu->item_count >= current_menu->item_capacity) { | ||
| size_t new_capacity = current_menu->item_capacity ? | ||
| current_menu->item_capacity * 2 : 16; | ||
| E9GuiMenuItem *new_items = realloc(current_menu->items, | ||
| new_capacity * sizeof(E9GuiMenuItem)); | ||
| if (!new_items) continue; /* Skip this item on failure */ | ||
| current_menu->items = new_items; | ||
| current_menu->item_capacity = new_capacity; | ||
| } | ||
| E9GuiMenuItem *item = ¤t_menu->items[current_menu->item_count++]; | ||
| memset(item, 0, sizeof(E9GuiMenuItem)); | ||
| strncpy(item->label, label, sizeof(item->label) - 1); | ||
| strncpy(item->command, command, sizeof(item->command) - 1); | ||
| item->enabled = true; | ||
| } else if (s[0] == '-') { | ||
| /* Separator */ | ||
| if (current_menu->item_count >= current_menu->item_capacity) { | ||
| size_t new_capacity = current_menu->item_capacity ? | ||
| current_menu->item_capacity * 2 : 16; | ||
| E9GuiMenuItem *new_items = realloc(current_menu->items, | ||
| new_capacity * sizeof(E9GuiMenuItem)); | ||
| if (!new_items) continue; /* Skip this item on failure */ | ||
| current_menu->items = new_items; | ||
| current_menu->item_capacity = new_capacity; | ||
| } | ||
| E9GuiMenuItem *item = ¤t_menu->items[current_menu->item_count++]; | ||
| memset(item, 0, sizeof(E9GuiMenuItem)); | ||
| strncpy(item->label, "-", sizeof(item->label) - 1); | ||
| } |
There was a problem hiding this comment.
Multiple strncpy calls in this function don't ensure null-termination. While the buffers are zero-initialized with memset, if the source strings are equal to or longer than the destination buffer size, the strings won't be null-terminated. Lines 689, 711, 712, and 727 should explicitly null-terminate after strncpy to ensure safety: buffer[sizeof(buffer) - 1] = '\0';
String Safety: - Add explicit null-termination after all strncpy calls to ensure strings are properly terminated even when source equals buffer size - Affected: window titles, panel IDs/titles, file paths, menu labels, menu commands, build configuration strings Error Handling: - Change buffer_ensure_gap to return int (0 success, -1 failure) instead of silently failing on allocation errors - Update buffer_insert to propagate allocation failures - Fix e9editor_get_selection to return initialized empty string instead of uninitialized memory for stub implementation All self-tests pass after changes.
Completed Tasks
Created e9studio_tedit.h - Integration header bridging tedit-cosmo with e9studio
Created e9studio_app.c - Application core with:
Gap buffer implementation for efficient text editing
Editor state management (cursor, selection, history)
File operations with language detection
Menu system with INI configuration support
Created e9studio_gui_main.c with:
Auto-detection of appropriate mode based on environment
--gui, --tui, --cli command-line options
Self-test diagnostics (--self-test)
Version and help information
Created platform/e9gui_cli.c with:
Text-based interface for binary analysis commands
Build system integration with variable substitution ({e}, {n}, {p})
Placeholder hooks for future analysis engine integration
Created Makefile.wasm with:
Build WAT files to WASM using binaryen's wasm-as
Optimize WASM files using wasm-opt
Example payload generation
Updated Makefile.e9studio with new targets:
make gui - Build e9studio-gui.com
make cli - Alias for gui target
make wasm - Build WASM payloads
make binaryen - Build binaryen tools
Test Results
=== E9Studio Self-Test ===
=== Results: 7 passed, 0 failed ===
The changes have been committed and pushed to claude/cosmopolitan-ide-wasm-port-gSYQg. And we are now submitting for review a PR to main.