Skip to content

Harden GUI bounds checks for release builds#267

Open
idrassi wants to merge 4 commits into
frang75:mainfrom
idrassi:fix/gui-security-bounds
Open

Harden GUI bounds checks for release builds#267
idrassi wants to merge 4 commits into
frang75:mainfrom
idrassi:fix/gui-security-bounds

Conversation

@idrassi
Copy link
Copy Markdown
Contributor

@idrassi idrassi commented May 16, 2026

Summary

This PR fixes four security-relevant GUI hardening issues identified during a review:

  • Validate layout_create dimensions before multiplying row/column counts and allocating cells.
    Invalid zero or overflowing dimensions now return NULL, making the invalid-input behavior explicit at the public API boundary.
  • Guard SplitView panel accumulation before writing into the fixed-size panel stack.
    This prevents a 33rd write past the stack array and also fixes the latent debug off-by-one where the legitimate 32nd panel previously triggered an assertion.
  • Reject invalid combo/popup selection indices before indexing item arrays or updating bound state.
  • Reject unknown DBind object types or member names in _gbind_field_modify.

Security Impact

These changes replace assertion-only assumptions with runtime checks in release builds. They address potential memory-safety failures including integer-overflow-driven out-of-bounds access, stack buffer overflow, invalid event index access, and DBind member lookup with UINT32_MAX.

@frang75
Copy link
Copy Markdown
Owner

frang75 commented May 17, 2026

Hi @idrassi

Reviewing your latest changes, I would like to share with you a fundamental part of the NAppGUI design regarding the API contract. NAppGUI features a strong API contract, based on preconditions, verified through thousands of assertions. For example:

Layout *layout_create(const uint32_t ncols, const uint32_t nrows)
{
    Layout *layout = heap_new0(Layout);
    uint32_t i, ncells = nrows * ncols;
    cassert(ncells > 0);
    layout->cells = arrpt_create(Cell);
    ...

A zero-size layout is not a normal runtime scenario, but rather a programmer bug. By immediately failing through assertions, it detects errors at the exact point where they occur and prevents the program from continuing in an inconsistent state. Furthermore, it simplifies the API because it guarantees that the returned value is always valid, eliminating redundant NULL checks throughout the client code. However:

Layout *layout_create(const uint32_t ncols, const uint32_t nrows)
{
    Layout *layout = NULL;
    uint32_t i, ncells = 0;
    cassert(ncols > 0);
    cassert(nrows > 0);
    if (ncols == 0 || nrows == 0)
        return NULL;

    cassert(nrows <= UINT32_MAX / ncols);
    if (nrows > UINT32_MAX / ncols)
        return NULL;

This is masking a programming error, making it difficult to debug, or forcing the programmer to constantly check that the returning layout is not NULL.

This can be extrapolated to all API input parameters: Strings with null content, out-of-index elements, impossible parameters, for example:

real32_t bmath_sqrtf(const real32_t value)
{
    cassert(value >= 0.f);
    return (real32_t)sqrtf((float)value);
}

A crash is preferable to:

real32_t bmath_sqrtf(const real32_t value)
{
    if (value > 0)    
        return (real32_t)sqrtf((float)value);
    return 0;
}

Therefore, it's important to differentiate between the concept of vulnerability and the concept of API misuse. NAppGUI will crash if misused, but it will always issue an assertion that we can link to a log or use to force the debugger to stop.

It is far less tolerable to maintain the preconditions and then perform defensive programming, since looking at the code already reveals that one of the two is unnecessary.

cassert(ncols > 0);
cassert(nrows > 0);
if (ncols == 0 || nrows == 0)
    return NULL;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants