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

optimize option in gif animation causes segfault #415

Closed
sfeam opened this issue Nov 14, 2017 · 4 comments
Closed

optimize option in gif animation causes segfault #415

sfeam opened this issue Nov 14, 2017 · 4 comments
Milestone

Comments

@sfeam
Copy link

sfeam commented Nov 14, 2017

libgd 2.2.5
In gd_gif_out::gdImageGifAnimAddCtx we find this comment:

            /* create optimized animation.  Compare this image to
               the previous image and crop the temporary copy of
               current image to include only changed rectangular
      [snip]
              Images should be of same size.  If not, a temporary
               copy is made with the same size as previous image.
             */

The "should be" remark is true, but in fact no such copy is made.
As a result, whenever (tim->sy > prev_tim->sy) the code segfaults while trying to compare pixels.

De-selecting optimization avoids the bad code and image generation proceeds normally.
The problem was found while tracing a segfault generated by calls into libgd from gnuplot.
https://sourceforge.net/p/gnuplot/bugs/1992/
It is 100% reproducible on multiple platforms, but I have not tried to construct a small artificial demonstration code not involving gnuplot.

@cmb69
Copy link
Contributor

cmb69 commented Nov 15, 2017

Thanks for reporting this issue. Reproducer:

#include <gd.h>


int main()
{
    gdImagePtr im1, im2;
    FILE *fp;

    im1 = gdImageCreate(100, 100);
    gdImageColorAllocate(im1, 0, 0, 0);
    im2 = gdImageCreate(10, 10);
    gdImageColorAllocate(im2, 255, 255, 255);

    fp = fopen("./415.gif", "wb");
    gdImageGifAnimBegin(im1, fp, 0, 0);
    gdImageGifAnimAdd(im1, fp, 1, 0, 0, 100, 1, NULL);
    gdImageGifAnimAdd(im2, fp, 1, 0, 0, 100, 1, im1);
    // replacing `im2` with `NULL` in the following line succeeds
    gdImageGifAnimAdd(im1, fp, 1, 0, 0, 100, 1, im2);
    gdImageGifAnimEnd(fp);
    fclose(fp);

    gdImageDestroy(im1);
    gdImageDestroy(im2);

    return 0;
}

It seems to me that making a resized copy of the image would be wrong, as well as assuming that TopOfs and LeftOfs are zero. At least this behavior would have to be documented.

@sfeam
Copy link
Author

sfeam commented Nov 15, 2017

I was thinking that a straighforward fix would start by checking for new image size larger than old image size, and in that case skipping the whole code block in favor of replacing the entire pixel array.

@pierrejoye pierrejoye added this to the GD 2.3.3 milestone Aug 25, 2021
@pierrejoye
Copy link
Contributor

Please check #736

pierrejoye added a commit that referenced this issue Aug 31, 2021
Fix #415, Assuming TopOfs and LeftOfs zero, we can safely skip any (x…
@sfeam
Copy link
Author

sfeam commented Oct 14, 2021

I re-ran the gnuplot script for which the "optimize" option caused a segfault, using libgd built from current git master.
It did not segfault, so that's good.
But it also did not create a correct plot. Every frame created was solid black (not the background color).
Running the script without "optimize" produces a correct animation, as before.

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

No branches or pull requests

3 participants