Walkthrough

Adds runtime verification to functable initialization: init_functable now returns int, force_init and many stubs return int, assignments use FUNCTABLE_VERIFY_ASSIGN with a memory barrier, and FUNCTABLE_INIT checks force_init return; also unconditionally includes <stdio.h> in zbuild.h.

Changes

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Stub as stub (e.g. crc32_stub)
    participant Functable as init_functable
    participant Verify as FUNCTABLE_VERIFY_ASSIGN
    participant Barrier as MemoryBarrier

    Stub->>Functable: call FUNCTABLE_INIT (calls force_init)
    Functable->>Functable: set safe default `ft.force_init = force_init_empty`
    Functable->>Verify: verify & assign function pointers (per-pointer)
    alt any verify fails
        Verify-->>Stub: return non-zero (propagate error)
    else all verify succeed
        Verify->>Barrier: enforce memory barrier
        Barrier-->>Stub: return Z_OK (init succeeded)
    end

Possibly related PRs

• Don't build C-fallback functions that never get used on x86_64 #1984 — Modifies functable initialization and per-pointer assignment logic (init_functable and function-pointer setup), sharing direct code-level overlap.

Suggested labels

Architecture

Suggested reviewers

• nmoinvaz

Pre-merge checks and finishing touches

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Codecov Report

❌ Patch coverage is 62.50000% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.03%. Comparing base (db3f23f) to head (e2c1c1d).

@@             Coverage Diff              @@
##           develop    #1983       +/-   ##
============================================
+ Coverage    39.33%   82.03%   +42.69%     
============================================
  Files           73      161       +88     
  Lines         7911    13748     +5837     
  Branches      1280     3149     +1869     
============================================
+ Hits          3112    11278     +8166     
+ Misses        4555     1439     -3116     
- Partials       244     1031      +787     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I figured calling abort() would not be very nice to the application linking zlib-ng.
Implemented improved failure handling for deflateinit and inflateinit.

Suggestions for how to handle other exported functions directly? Currently I guess they'll likely segfault, same as they would without this PR.
Edit: Or they get infinite recursion like coderabbitai mentions. Its "Solution 2" might be ok for those calling functions directly..?

I figured calling abort() would not be very nice to the application linking zlib-ng. Implemented improved failure handling for deflateinit and inflateinit.

Suggestions for how to handle other exported functions directly? Currently I guess they'll likely segfault, same as they would without this PR. Edit: Or they get infinite recursion like coderabbitai mentions. Its "Solution 2" might be ok for those calling functions directly..?

Aborting with error message is still better than "Segmentation Fault" without any clue what went wrong... Solution 2 would most likely result in hidden data corruption as not all functions return status code.

Aborting with error message is still better than "Segmentation Fault" without any clue what went wrong...

Yeah, I agree.

Solution 2 would most likely result in hidden data corruption as not all functions return status code.

No, solution 2 is doing printf + abort for all the direct-calls, while deflateinit/inflateinit would still return Z_VERSION_ERROR (I figure that is the error message most similar, in this case indicating that the zlib version tring to run is not compiled to be compatible with the current cpu).
Inflateinit/deflateinit would never complete, and zlib-ng would never even get to a state of accepting data, so no data loss there.
Potential data loss for the direct calls that hit the abort() though, but as we agree, that is effectively no worse than a segfault anyways, and I don't see any other proper way to do it.

Again, this is an error condition that should never occur with a default build, only potentially possible with custom build configs where generic functions are left out as well as the needed optimized functions. Currently should not be possible with CMake/Configure without using custom defines or code changes. Even with #1984, this is not possible since it makes sure to require WITH_SSE2 and WITH_OPTIM before allowing the removal of the specific generic fallbacks that SSE2 can replace.

But I feel that it is better to have a defined failure mode IF this ever happens.

I'll work on a solution for the stub functions. I also think several of the stub functions should be unreachable (not directly callable and always after inflateinit/deflateinit), not sure whether we should keep those as functional unreachable stubs or replace them with a common one that just calls abort (not something for this PR in any case).

Edit: Implemented changes and squashed the commits, also improved commit message.

I'll work on a solution for the stub functions. I also think several of the stub functions should be unreachable (not directly callable and always after inflateinit/deflateinit), not sure whether we should keep those as functional unreachable stubs or replace them with a common one that just calls abort (not something for this PR in any case).

A lot of changes have been made with the stub functions since I initially designed the logic... I haven't been happy with all the changes and have already said so earlier... It might be best to do proper redesign/rewrite and not just commit random patches that cause more dead code. In the current state, some of the stub functions can be called, but others are essentially no-op in normal code. Coverity scan will get very low coverage due to stub calls being mutually exclusive.