Fix VipsDriver save() corrupting source on same-path writes#326
Merged
Conversation
… path libvips streams from the source while writing the result. When loadFile() and save() target the same path, libvips reads from the file as it's being overwritten, which can crash libjpeg-turbo with SIGBUS on JPEGs (see libvips/libvips#4397). When the destination matches originalPath, write to a sibling temp file and atomically rename over the destination. Also adds vips to the drivers test dataset so the existing matrix exercises it, with the CI workflow installing libvips and enabling FFI.
- Throw CouldNotSaveImage on rename or buffer-write failure rather than silently corrupting the destination - Make $originalPath nullable with a default of null instead of relying on isset() against an uninitialised typed property - Drop @Unlink, the surrounding file_exists check already guards it - Fold the dimension assertion into the existing dataset test and remove the duplicate vips-only test (the dataset addition already covers vips)
- Reset quality on loadFile() like GdDriver does, so calling quality() on one image doesn't bleed into a later image loaded on the same driver instance (this surfaced now that vips is in the shared dataset) - Drop the CannotOptimizePng throw and instead map quality (0-100) to PNG compression (0-9) on save, matching GdDriver's behavior
Vips uses Pango/Cairo for text, which produces a different rendering from the FreeType-based output the snapshots were captured against. Generating vips-specific snapshots would need a separate baseline captured on the same CI environment, which is out of scope here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
loadFile()andsave()target the same path, libvips reads from the file as it's being overwritten, which can crash libjpeg-turbo with SIGBUS on JPEGs (libvips/libvips#4397). When the destination matchesoriginalPath, write to a sibling temp file and atomically rename over the destination.vipsto thedriverstest dataset so the existing matrix exercises it (previously gd + imagick only, which is why the regression slipped through).Background
This was reported via spatie/laravel-medialibrary#3923. Medialibrary's conversion pipeline calls
loadFile($path)->...->save()(no path), which now resolves back to the load path due to PR #321. With'access' => 'sequential'the source is streamed during write, triggering the libvips bug. The reporter sees deterministic SIGBUS on Sail/Docker/WSL2; on macOS the same call silently produces an altered file (different MD5 from the same input).The fix is local to
VipsDriver::save()so every consumer (medialibrary or otherwise) is protected without needing changes downstream.