Skip to content

Improve memory safety #118

@jserv

Description

@jserv

he codebase demonstrates generally good memory safety practices but has critical vulnerabilities in the callback/closure system.
Key findings:

  • 89 malloc/calloc calls, 47 free calls - consistent allocation patterns
  • Primary vulnerability: Work queue closure validation missing in src/work.c

Proper cleanup chain in twin_window_destroy() (src/window.c)

  • Calls destroy callback if set
  • Destroys pixmap with twin_pixmap_destroy()
  • Frees window name string
  • Frees window structure
  • Toplevel cleanup: Proper cleanup in _twin_toplevel_destroy() with event dispatch
  • Pixmap cleanup: Consistent use of twin_pixmap_destroy() across 15+ call sites

Event Handling and Work Queue Pointer Safety

  • Work queue execution: _twin_run_work() (src/work.c) lacks closure validation
  • Potential issue:** Work items can be queued with closures that become invalid
  • No safety checks: Missing validation in twin_set_work() for closure pointer validity
  • Queue deletion: Uses deferred deletion pattern with deleted flag (src/queue.c)

Buffer Overflow Risks

  • String functions: Limited use of dangerous functions
    • strlen() + malloc() pattern used consistently (4 occurrences)
    • strcpy() used safely after proper allocation (2 occurrences)
    • No use of gets(), scanf(), or other dangerous functions
  • Image loading:
    • GIF decoder: Uses proper bounds checking in read_num() and palette handling
    • PNG loader: Uses libpng with proper error handling and cleanup
    • TVG loader: Validates dimensions before allocation
  • Buffer allocations: All image buffers allocated with calculated sizes

Use-After-Free in Callback Systems

  • Work queue closures: Referenced widgets may be destroyed while work items are queued
  • Timeout callbacks: Similar risk with timeout closures (src/timeout.c)
  • Custom widget callbacks: Registration map may retain stale pointers after widget destruction
  • Event callbacks: Window event callbacks may reference freed data
  • _twin_toplevel_paint() and _twin_toplevel_layout() callbacks use top-level pointer without validation
  • twin_set_timeout() accepts closure without lifetime management
  • Custom widget dispatch map cleanup occurs in unregister_custom_widget() but timing may be incorrect

Expected output: overcome the above risks.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions