Re: Bug#846385: imagemagick: Potential ABI break upstream (without SONAME change)

[roucaries bastien]

BTW feel free to NMU imagemagick during a short break I take in the
next two days.

Bastien

Re: Bug#846385: imagemagick: Potential ABI break upstream (without SONAME change)

[roucaries bastien]

On Tue, Dec 20, 2016 at 4:22 PM, roucaries bastien
 wrote:
> On Wed, Dec 14, 2016 at 1:29 PM, roucaries bastien
>  wrote:
>> On Wed, Dec 14, 2016 at 1:28 PM, roucaries bastien
>>  wrote:
>>> On Tue, Dec 13, 2016 at 12:21 AM, Emilio Pozuelo Monfort
>>>  wrote:
 On 09/12/16 22:37, roucaries bastien wrote:
> control: forwarded -1 
> https://github.com/ImageMagick/ImageMagick/issues/320
>
> Dear realease team,
>
> What is the next step?

 In which version was the ABI break introduced?
>>>
>>> It was introduced more than 2 years ago ( 6.9.2-10). One version after
>>> jessie what lie in unstable before jessie release.

 In general I would prefer the change to be reverted, but depending on how 
 long
 this has been in the archive, and in order to stay up to date for security
 fixes, it may be best to do the soname bump.
>>>
>>> From a security point of view, I prefer recent version. I do not want
>>> to keep jessie version with huge patch queue for

 Can you check if your rdeps build fine against a new imagemagick?
>
> libmagick++ rdeps build fine except traficserver due to a sphinx error
> (unreleated to imagemagick)
> libmagickwand rdeps build fine except rss-glx due to unreleated build
> conflict (#838800)
> libmagickcore rdeps build fine except dx due to missing .mak file
> (unlikely imagemagick)

trafficserver is - #848800
dx #848894

>
> Will rebuild traficserver and dx under sid chroot and report FTBFS.
>
> Seems it is ok
>
> Bastien
>>>
>>> What i will do i will set on unstable the newer version with so dump
>>> and will begin to rebuilt on pbuilder. Normally it will be fine.
>>
>> s/unstable/experimental/g
>>>
>>> I wish to have abi checker on the debian side
>>>
>>> Bastien

 Emilio

Re: Bug#846385: imagemagick: Potential ABI break upstream (without SONAME change)

[roucaries bastien]

On Wed, Dec 14, 2016 at 1:29 PM, roucaries bastien
 wrote:
> On Wed, Dec 14, 2016 at 1:28 PM, roucaries bastien
>  wrote:
>> On Tue, Dec 13, 2016 at 12:21 AM, Emilio Pozuelo Monfort
>>  wrote:
>>> On 09/12/16 22:37, roucaries bastien wrote:
 control: forwarded -1 https://github.com/ImageMagick/ImageMagick/issues/320

 Dear realease team,

 What is the next step?
>>>
>>> In which version was the ABI break introduced?
>>
>> It was introduced more than 2 years ago ( 6.9.2-10). One version after
>> jessie what lie in unstable before jessie release.
>>>
>>> In general I would prefer the change to be reverted, but depending on how 
>>> long
>>> this has been in the archive, and in order to stay up to date for security
>>> fixes, it may be best to do the soname bump.
>>
>> From a security point of view, I prefer recent version. I do not want
>> to keep jessie version with huge patch queue for
>>>
>>> Can you check if your rdeps build fine against a new imagemagick?

libmagick++ rdeps build fine except traficserver due to a sphinx error
(unreleated to imagemagick)
libmagickwand rdeps build fine except rss-glx due to unreleated build
conflict (#838800)
libmagickcore rdeps build fine except dx due to missing .mak file
(unlikely imagemagick)

Will rebuild traficserver and dx under sid chroot and report FTBFS.

Seems it is ok

Bastien
>>
>> What i will do i will set on unstable the newer version with so dump
>> and will begin to rebuilt on pbuilder. Normally it will be fine.
>
> s/unstable/experimental/g
>>
>> I wish to have abi checker on the debian side
>>
>> Bastien
>>>
>>> Emilio

Re: Bug#846385: imagemagick: Potential ABI break upstream (without SONAME change)

[roucaries bastien]

On Wed, Dec 14, 2016 at 1:28 PM, roucaries bastien
 wrote:
> On Tue, Dec 13, 2016 at 12:21 AM, Emilio Pozuelo Monfort
>  wrote:
>> On 09/12/16 22:37, roucaries bastien wrote:
>>> control: forwarded -1 https://github.com/ImageMagick/ImageMagick/issues/320
>>>
>>> Dear realease team,
>>>
>>> What is the next step?
>>
>> In which version was the ABI break introduced?
>
> It was introduced more than 2 years ago ( 6.9.2-10). One version after
> jessie what lie in unstable before jessie release.
>>
>> In general I would prefer the change to be reverted, but depending on how 
>> long
>> this has been in the archive, and in order to stay up to date for security
>> fixes, it may be best to do the soname bump.
>
> From a security point of view, I prefer recent version. I do not want
> to keep jessie version with huge patch queue for
>>
>> Can you check if your rdeps build fine against a new imagemagick?
>
> What i will do i will set on unstable the newer version with so dump
> and will begin to rebuilt on pbuilder. Normally it will be fine.

s/unstable/experimental/g
>
> I wish to have abi checker on the debian side
>
> Bastien
>>
>> Emilio

Re: Bug#846385: imagemagick: Potential ABI break upstream (without SONAME change)

[roucaries bastien]

On Tue, Dec 13, 2016 at 12:21 AM, Emilio Pozuelo Monfort
 wrote:
> On 09/12/16 22:37, roucaries bastien wrote:
>> control: forwarded -1 https://github.com/ImageMagick/ImageMagick/issues/320
>>
>> Dear realease team,
>>
>> What is the next step?
>
> In which version was the ABI break introduced?

It was introduced more than 2 years ago ( 6.9.2-10). One version after
jessie what lie in unstable before jessie release.
>
> In general I would prefer the change to be reverted, but depending on how long
> this has been in the archive, and in order to stay up to date for security
> fixes, it may be best to do the soname bump.

>From a security point of view, I prefer recent version. I do not want
to keep jessie version with huge patch queue for
>
> Can you check if your rdeps build fine against a new imagemagick?

What i will do i will set on unstable the newer version with so dump
and will begin to rebuilt on pbuilder. Normally it will be fine.

I wish to have abi checker on the debian side

Bastien
>
> Emilio

Re: Bug#846385: imagemagick: Potential ABI break upstream (without SONAME change)

[Emilio Pozuelo Monfort]

On 09/12/16 22:37, roucaries bastien wrote:
> control: forwarded -1 https://github.com/ImageMagick/ImageMagick/issues/320
> 
> Dear realease team,
> 
> What is the next step?

In which version was the ABI break introduced?

In general I would prefer the change to be reverted, but depending on how long
this has been in the archive, and in order to stay up to date for security
fixes, it may be best to do the soname bump.

Can you check if your rdeps build fine against a new imagemagick?

Emilio

Re: 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  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  (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

Re: Bug#846385: imagemagick: Potential ABI break upstream (without SONAME change)

[Simon McVittie]

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  (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