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

Double free problem #673

Merged
merged 2 commits into from Aug 25, 2019
Merged

Double free problem #673

merged 2 commits into from Aug 25, 2019

Conversation

os97673
Copy link
Contributor

@os97673 os97673 commented Aug 10, 2019

If a gif has at least three frames, first with non-zero size and two others with 0 size then the lib may free one pointer twice.

I'm not sure if I've correctly understood the code but it looks like it is enough to just free rasterBits and nullify them in case of empty frame.

Also I've defined LOGE() macro for release (as NOOP) to avoid #ifdef DEBUG every time I need to use logging ;)

The version of problematic gif added (since it is rather strange gif I've zipped it to avoid possible problems with its playback.
exploit.zip

This way it is much easier to user logging.
@codecov-io
Copy link

codecov-io commented Aug 10, 2019

Codecov Report

No coverage uploaded for pull request base (dev@ca8a4a3). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##             dev    #673   +/-   ##
=====================================
  Coverage       ?   4.48%           
=====================================
  Files          ?      26           
  Lines          ?    1294           
  Branches       ?     108           
=====================================
  Hits           ?      58           
  Misses         ?    1233           
  Partials       ?       3

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca8a4a3...4944c92. Read the comment docs.

if realloc() is called with 0 size it may return NULL and this will be incorrectly handled
as not enough memory and (also) rasterBits will be freed by realloc but we will not update
it.
Copy link
Owner

@koral-- koral-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM

@koral--
Copy link
Owner

koral-- commented Aug 25, 2019

I've analyzed this case deeper and it turns out that frames having 0 size are ignored when reading file using DGifGetCodeNext (this is used just after opening file to determine number of frames etc.) but they are not ignored when using DGifGetLine (this is used when decoding pixels).

Although this change fixes the issue visible to user I'll report this inconsistency to GIFLib bugtracker. I expect that while reading file in both ways we should see the same number of frames.

@koral-- koral-- merged commit cb137e8 into koral--:dev Aug 25, 2019
@davidweigao
Copy link

Hi @koral-- , when will be the next release that contains this fix?

@koral--
Copy link
Owner

koral-- commented Oct 2, 2019

It is already released about month ago.

@davidweigao
Copy link

Ah it's in 1.2.18. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

4 participants