Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libc (malloc?) crash when returning false in true streaming custom target #4178

Open
L3tum opened this issue Oct 2, 2024 · 8 comments
Open
Labels

Comments

@L3tum
Copy link

L3tum commented Oct 2, 2024

Bug report

Describe the bug
This may just be me misusing the API, but I'm not sure how else do to what I need to do, and it should™ work from what I understand. When you write a custom target for streaming (in PHP) and return false from the onWrite call (similar to what fwrite would do when the write fails), then you'll get an Exception from libvips that doesn't seem well handled in the first place (I'd guess an exception passed through from the underlying library, because it contains a different exception message depending on the target image format, for example). Shortly after receiving that exception, libc (sometimes™, around 80% of the time I'd guess) crashes with an error related to malloc (usually doubly-linked list corrupted).
I'm not at a capable PC right now, so it could be the case this is just PHP FFI and other languages handle it better (i.e. without crashing).

To Reproduce
Steps to reproduce the behavior:

  1. (in the PHP bindings at least, maybe other bindings) Write a custom target that returns false at some point
  2. Initiate an image transformation of some sort
  3. Notice exception being thrown
  4. (If you handle the exception (through try/catch) then (potentially on next call to libvips) the libc will crash
  5. This is usually with the message double-linked list corrupted which is apparently related to malloc. There was also a double-free error once. A coredump is generated as well

Expected behavior
Libvips should ideally handle the response stream stopping without crashing libc, and potentially without throwing an exception (since it's unexpected) or at least document the exception and handle the underlying exception by wrapping it in a more general vips exception (like "response stream broken" rather than the image format specific messages). It should also be documented how to gracefully stop the streaming ideally from the custom target itself (since that would be the location where, for example, a client going away would be handled).

Actual behavior
Image format specific message in exception coupled with a crashing libc

Screenshots
Here's the exception message for jpeg for example:

libvips error: target_custom: write error
unix error: Success
VipsJpeg: Output file write error --- out of disk space?

For PNG it's:

libvips error: target_custom: write error
unix error: Success 
pngsave_target: stream error
wbuffer_write: write failed
unix error: Unknown error -1

Environment
(please complete the following information)

  • OS: Debian Bookworm
  • Vips: Latest (8.15.3)

Additional context
I couldn't identify all exception messages, since some of them just say libvips error and that's it. So I couldn't confirm if some image formats actually handle this case and libvips crashes, or if none of them handle it. I've also had a weird behaviour where the next call to libvips would return the result of the previous (completely unrelated) call for generating an image, but I haven't identified where that may be coming from, so is just meant as additional info that may help you. I will try to write a small reproducer, but I'm writing this bug report from my phone (cause no capable PC as I just moved).

@L3tum L3tum added the bug label Oct 2, 2024
@jcupitt
Copy link
Member

jcupitt commented Oct 2, 2024

Hi @L3tum,

I tried to make a reproducer:

#!/usr/bin/env php
<?php

require dirname(__DIR__) . '/vendor/autoload.php';

use Jcupitt\Vips;

if (count($argv) != 4) {
    echo "usage: $argv[0] IN-FILE OUT-FILE FORMAT\n";
    echo "  eg.: $argv[0] ~/pics/k2.jpg x.tif .tif[tile,pyramid]\n";
    exit(1);
}

$in_file = fopen($argv[1], 'r');
$source = new Vips\SourceCustom();
$source->onRead(function ($bufferLength) use (&$in_file) {
    // return 0 for EOF, -ve for read error
    return fread($in_file, $bufferLength);
});
// seek is optional
$source->onSeek(function ($offset, $whence) use (&$in_file) {
    if (fseek($in_file, $offset, $whence)) {
        return -1;
    }

    return ftell($in_file);
});

// open for write and read ... formats like tiff need to be able to seek back
// in the output and update bytes later
$out_file = fopen($argv[2], 'w+');
$out_count = 0;  
$target = new Vips\TargetCustom();
$target->onWrite(function ($buffer) use (&$out_file, &$out_count) {
    $result = fwrite($out_file, $buffer);

    $out_count += 1;
    if ($out_count > 10) {
        return -1;
    }
    
    if ($result === false) {
        // IO error
        return -1;
    } else {
        return $result;
    }
});
// read and seek are optional
$target->onSeek(function ($offset, $whence) use (&$out_file) {
    if (fseek($out_file, $offset, $whence)) {
        return -1;
    }

    return ftell($out_file);
});
$target->onRead(function ($bufferLength) use (&$out_file) {
    return fread($out_file, $bufferLength);
});

$image = Vips\Image::newFromSource($source);

for ($i = 0; $i < 10; $i++) { 
    echo "loop ", $i, "\n";
    try {
        $image->writeToTarget($target, $argv[3]);
    } catch (Exception $e) {
        echo "Caught exception: ",  $e->getMessage(), "\n";
    }
}

But it seems to work fine, on ubuntu 24.04 at least. I see:

$ ./streaming-custom.php ~/pics/k2.jpg x.tif .tif
loop 0
Caught exception: libvips error: target_custom: write error
unix error: Success
TIFFAppendToStrip: Write error at scanline 1280
wbuffer_write: write failed
unix error: Unknown error -1
target_custom: write error
unix error: Success
TIFFWriteDirectoryTagData: IO error writing tag data
target_custom: write error
unix error: Success

loop 1
Caught exception: libvips error: target_custom: write error
unix error: Success
target_custom: write error
unix error: Success
TIFFAppendToStrip: Maximum TIFF file size exceeded
wbuffer_write: write failed
unix error: Unknown error -1
target_custom: write error
unix error: Success
target_custom: write error
unix error: Success
target_custom: write error
unix error: Success
TIFFWriteDirectoryTagData: IO error writing tag data
target_custom: write error
unix error: Success
...

Though unix error: Success is probably a very minor bug.

Could you make a small program I can run that shows the error?

@jcupitt
Copy link
Member

jcupitt commented Oct 2, 2024

That code was adapted from php-vips/examples/streaming-custom.php fwiw.

@jcupitt
Copy link
Member

jcupitt commented Oct 2, 2024

Write a custom target that returns false at some point

Ah maybe that's all the problem is? You need to return -1 for an error, just like C's fwrite(). Looking at the docs I agree it's ambiguous:

https://libvips.github.io/php-vips/classes/Jcupitt-Vips-TargetCustom.html#method_onWrite

Since there's a fwrite() in php as well. I'll update it to be more explicit.

@jcupitt
Copy link
Member

jcupitt commented Oct 2, 2024

Updated docs: libvips/php-vips@5f8ed10

@L3tum
Copy link
Author

L3tum commented Oct 2, 2024

Hahaha, you were faster than me. Thanks for all the work!

Since I wrote the docs in php-vips myself I feel kind of stupid now. Can you check how it's handled in pyvips? I'm pretty sure I looked at the typings of that to figure out what should and shouldn't be expected in PHP, so maybe the docs there are ambiguous as well.

@kleisauke
Copy link
Member

I wonder if this is related to commit php/php-src@f547412, i.e. the Throwing from FFI callbacks is not allowed-error. Perhaps a TypeError is thrown due to strict typing being enabled (declare(strict_types=1))?

You need to return -1 for an error, just like C's fwrite().

I think in some cases we also treat 0 as an error, see for example:

/* n == 0 isn't strictly an error, but we treat it as
* one to make sure we don't get stuck in this loop.
*/
if (bytes_written <= 0) {

(assuming false is automagically coerced to 0 in PHP)

@jcupitt
Copy link
Member

jcupitt commented Oct 2, 2024

Yes, you cannot throw across FFI in any language, I think. It has to be error codes.

Maybe FFI::gobject()->g_value_set_int64() and friends should test the argument type?

@jcupitt
Copy link
Member

jcupitt commented Oct 2, 2024

Can you check how it's handled in pyvips?

Yes, it's -1 in pyvips too. The -1 is a libvips thing, it's not part of the binding, so it'll be the same everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants
@jcupitt @L3tum @kleisauke and others