[Pkg-gmagick-im-team] Bug#846385: imagemagick: Potential ABI break upstream (without SONAME change)

[roucaries bastien]

control: forwarded -1 https://github.com/ImageMagick/ImageMagick/issues/320

Dear realease team,

What is the next step?

Thank you

On Tue, Dec 6, 2016 at 8:23 PM, Simon McVittie <smcv at debian.org> wrote:
> Control: reopen 846385
> Control: severity 846385 serious
>
> tl;dr: yes, this is clearly an ABI break.
>
> On Thu, 01 Dec 2016 at 15:55:02 +0100, roucaries bastien wrote:
>> * struct _DrawInfo (1) is not a problem from a C point of view because
>> it should be set and destry by API function. It is a opaque object. So
>> no need to so bump for this
>> *  _ElementReference (1) is part of _Drawinfo so not a problem
>> * _GradientInfo (3) is the same
>> * For _image is an opaque type so it is the same
>
> Unfortunately this is not true: none of these types are opaque. I think
> you have misunderstood what it means to say a struct type is opaque.
> For example, DrawInfo (aka struct _DrawInfo) has its layout visible to
> library users in the installed header <magick/draw.h> (in
> libmagickcore-6-headers), so any change to its size is an ABI break, and
> so is any semantic change to its contents.
>
> You are right to think that if library users were expected to allocate
> a DrawInfo on the stack, it would certainly be an ABI break to change
> its size. However, even if a structure is only ever allocated by
> library code, reading its fields directly is part of the library ABI too.
>
> For DrawInfo to be an opaque object, its layout would have to be
> in a .c or .h file used only internally by ImageMagick (and not installed
> in libmagickcore-6-headers), and library users would have to access its
> fields via accessor functions like DrawInfoGetGravity() and
> DrawInfoSetGravity(). For example, GRegex in GLib's glib/gregex.[ch]
> is a good example of an opaque object:
> https://sources.debian.net/src/glib2.0/2.50.2-2/glib/gregex.h/
> https://sources.debian.net/src/glib2.0/2.50.2-2/glib/gregex.c/
>
> The definition of an ABI break is that previously-correct code, compiled
> against version A, no longer works correctly when linked at runtime with
> version B. Consider what would happen if some program that uses ImageMagick
> is compiled against ImageMagick 6.9.1-10, and it wants to set the stroke
> opacity (which happens to be the last field in the struct):
>
>     DrawInfo *draw_info = AcquireDrawInfo();
>
>     draw_info->stroke_opacity = 0.5;
>
> In ImageMagick 6.9.1-10 on x86_64, according to the ABI Compliance Checker,
> the DrawInfo is a 720 byte struct. A double is 8 bytes, and stroke_opacity
> is the last thing in the struct, so that assignment results in an 8-byte
> write 712 bytes after the draw_info pointer, overwriting the bytes from
> 712 to 719 bytes after the pointer. In other words, on x86_64, it's the
> same machine code that you would get by compiling this:
>
>     DrawInfo *draw_info = AcquireDrawInfo();
>
>     *((double *) (((char *) draw_info) + 712)) = 0.5;
>
> Now suppose we install that program on a machine that has ImageMagick
> 6.9.2-10, in which DrawInfo has grown by 32 bytes. The program still
> writes at an offset of 712 bytes into the struct (remember that
> machine code doesn't know anything about structs or names, only
> pointers and offsets), but now the larger GradientInfo and
> ElementReference have pushed all the later struct fields further
> away from offset 0. If my arithmetic is correct, the field 712 bytes
> into the data structure is now draw_info->interword_spacing, which is
> not what was intended.
>
> Conversely, if you compile against 6.9.2-10 but use 6.9.1-10 at runtime,
> it's worse: the program writes at an offset of 724 bytes (overwriting
> bytes 724 to 731 inclusive); but since ImageMagick 6.9.1-10 only allocated
> a 720 byte struct, that write is outside the struct, and could be anything
> (in particular, it could be corrupting glibc malloc data structures).
>
> Regards,
>     S