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 <s...@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
