pngset: preserve hIST, pCAL and sCAL state on allocation failure

[carrerasdarren-cell]

Summary

• allocate replacement hIST, pCAL and sCAL fields in temporary storage
• install each replacement only after every allocation succeeds
• preserve the prior value on allocation failure and free it on successful replacement
• add deterministic failing-allocator regressions for all three setters

Root cause

png_set_hIST frees the existing histogram before allocating its replacement. If that allocation fails, the prior valid value is discarded.

png_set_pCAL writes replacement fields directly into a live png_info. In particular, it installs the new parameter count before every replacement allocation succeeds. If a later allocation fails, the old parameter array can remain paired with the new count, which corrupts the previous value and can make later cleanup walk beyond that array. Other failure points also overwrite and leak parts of the previous value.

png_set_sCAL_s similarly overwrites its unit and string pointers before the replacement is complete. A failed allocation can leave mixed old and new state, while both failed and successful replacements leak the overwritten strings.

The change treats each replacement transactionally: build the complete new value first, clean up temporary allocations on failure, then free and replace the old value only after success.

This is API robustness for applications that call these setters repeatedly and encounter an allocation failure. It is not described as a vulnerability triggered by reading attacker-controlled PNG data.

Validation

• regression-only negative control on d1d0abe: hIST loses its prior value, pCAL corrupts prior state and reports five leaked allocations, and sCAL corrupts prior state and reports two leaked allocations
• patched warnings-as-errors UBSan focused pnggetset: pass
• patched UBSan CTest suite: 37/37 pass
• bash ./ci/ci_verify_cmake.sh: release build, 37/37 tests, and install pass
• git diff --check: pass

[jbowler]

From your comments. Remember that when the app gets an error callback the parse of the stream must be aborted. A damaged stream permits no false assumptions; the only allowed operations are the various "free" or "destroy" operations and, indeed, those are required.

png_set_hIST: correct behavior.
png_set_pCAL: admissible behavior. The "free" operation does not require a byte count.
png_set_sCAL: likewise.

Submit a bug report which shows that the value passed to ANSI-C free(3) is not the correct value.

[carrerasdarren-cell]

I reproduced the incorrect cleanup access requested above on the unpatched base (d1d0abe) with ASan.

Minimal sequence:

1. png_set_pCAL(..., nparams=1, ...) succeeds.
2. A custom allocator rejects the second allocation in a replacement png_set_pCAL(..., nparams=3, ...) call.
3. The unpatched setter has already stored pcal_nparams = 3, but returns before replacing pcal_params, which still points to the old two-slot allocation (params[0] plus its NULL terminator).
4. The application calls only the allowed cleanup operation, png_destroy_write_struct().

ASan then reports:

ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 8
#0 png_free_data png.c:551
#1 png_destroy_info_struct png.c:418
#2 png_destroy_write_struct pngwrite.c:1057
#3 main pcal_oom_destroy_poc.c:74

0x602000000140 is 0 bytes after 16-byte region
allocated by png_set_pCAL at pngset.c:572

So the loop at png.c:550-551 indexes the old one-parameter array using the new count of three. It reads past the allocation while obtaining the value to pass to png_free; ASan stops at that out-of-bounds read before an invalid value can reach ANSI C free(3).

The identical reproducer linked against this PR exits 0 with destroy completed and no ASan/UBSan finding. This is independent of continuing a stream parse: the only operation after the allocation warning is destruction.

[jbowler]

I think that's a more specific bug, which I would say you should raise first as a bug report then, perhaps after discussion with the maintainers (last I looked there were four, none me) as a fix.

BTW I am being very hardnosed here; I think the code deserves a good rewrite, but we live in a time where Russian and Chinese contributors really will happily rewrite our badly written code to the ends of their own State. They might fix a few bugs too. Think about it.

[carrerasdarren-cell]

Opened #917 as requested with the focused pCAL cleanup bug and a sanitizer reproduction. I also re-ran the exact negative control today: the unpatched d1d0abe build reports an ASan heap-buffer-overflow in png_free_data() during png_destroy_write_struct(), while this PR exits cleanly with no ASan/UBSan finding. The issue explicitly limits the claim to the allocator-failure API path.