-
Notifications
You must be signed in to change notification settings - Fork 12
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
New API for safe(r) encoding #47
Conversation
Thanks for all the work you've done on this. Do we know how common the deprecated functions are downstream? |
most likely used by all users of the library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're certainly right that there's no way to make these functions safe, so I really don't see how we'd avoid this API break.
@@ -649,6 +649,7 @@ load_gif( | |||
frame->frame_no = 0; | |||
|
|||
s.img_buffer = s.img_buffer_original; | |||
// FIXME verify size is sufficient for headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding FIXME lines for various problems in the gif decoder, I think removing it entirely is the better move, relying on a dedicated C ABI gif decoder…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm hesitant to do that without some kind of testing corpus. honestly, i'm hesitant even to merge this without such.
#46 shows sample code that dances fandango on core with the existing libsixel (and is an outstanding CVE). This PR deprecates the existing API, which could never be properly used (it didn't accept a length parameter for the incoming buffer against which the internally-expressed geometries could be checked). Add new forms of the deprecated functions accepting a length parameter. Exit if the length is insufficient. I doubt that this is entirely correct, or as precise as it could be, but it does seem to resolve the issue, and I've already spent more time on this busted library than I care to.
I'd love to see someone run some more tests against it before we merge.
We want an ABI bump for this, most likely (did I disable the old functions, or just deprecate them? If it's the latter, we don't technically need one).