InsightSoftwareConsortium / InsightSoftwareConsortium/ITK

PNGImageIO::WriteSlice: destroys existing file on mid-stream failures, leaks libpng structs on error, rowInc overflow for extreme widths

Open
#6,618 0 comments 0 reactions 0 assignees View on GitHub
type:Bug
Dominant language
C++
Stars
1.7k
Forks
748
Avg merge
1d 1h
Merged PRs (30d)
64

Description

Three pre-existing defects remain in `PNGImageIO::WriteSlice` after the early-validation fixes in #6614 (B70/B72 of #6575). All are in error paths that run after the output file has been opened, or in row-stride arithmetic.

1. Existing file still destroyed on mid-stream write failures

`PNGFileWrapper pngfp(fileName.c_str(), "wb")` truncates any existing file at the destination path. #6614 moved the component-type and component-count validation ahead of this open, but every failure mode after it still destroys the prior file:

- `png_create_write_struct` / `png_create_info_struct` failure
- any libpng error reaching the `setjmp` handler (e.g. disk full during `png_write_image`)

In those cases an exception is thrown *and* the original file has been replaced by an empty or partial PNG. A complete fix likely means writing to a temporary file and renaming into place on success.

2. setjmp error path leaks libpng structs (and palette buffer)

The `setjmp(png_jmpbuf(png_ptr))` branch throws via `itkExceptionMacro` without calling `png_destroy_write_struct(&png_ptr, &info_ptr)`, and when palette writing is enabled the `png_malloc`'d palette is never `png_free`'d. Every libpng write error leaks the write/info structs; an application that retries failed writes (e.g. a series writer looping slices onto a full disk) leaks on each attempt.

3. rowInc integer overflow for extreme image widths

`rowInc = width * numComp * bitDepth / 8` is computed in (unsigned then narrowed to) `int`. PNG allows widths up to 2^31-1, so a 16-bit RGBA slice with width ≥ 2^28 overflows `rowInc`, producing wrong `row_pointers` strides and out-of-bounds reads in `png_write_image`. The computation should use `size_t`/`SizeValueType` throughout.

Contributor guide

Open the contributing guide

Research direction

Read Modules/IO/PNG/src/itkPNGImageIO.cxx around WriteSlice, especially the file open near line 535, setjmp handler near line 561, and rowInc near line 675. Trace each failure path and verify that failed writes preserve the destination, libpng resources and palette memory are released, and row-stride arithmetic remains safe for extreme widths.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
computer-vision
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.