[PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[Robin Lee]

On XenServer 7.1.1, we start a vm with XAPI but attach a block device with xl.
We create an empty json config for the vm with the content "{}\n" and then
run 'xl block-attach':

  #  xl block-attach 1 phy:/dev/loop0 xvdz w
  libxl: error: libxl_json.c:950:libxl__json_parse: yajl error: parse error: trailing garbage
                                      {} K]
                       (right here) ------^

  libxl: error: libxl_json.c:1053:libxl__object_from_json: unable to generate libxl__json_object from JSON representation of libxl_domain_config.
  libxl: error: libxl.c:1995:device_addrm_aocomplete: unable to add device
  libxl_device_disk_add failed.

After investigation, we found the buffer returned from libxl_read_file_contents
is not null-terminated. But later in libxl__object_from_json, the buffer is expected to
be null-terminated. So parsing may exceeded the end of file and get in to uninisialized
momery area.

Signed-off-by: Robin Lee <robinlee.sysu@gmail.com>
---
 tools/libxl/libxl_utils.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 507ee56..51134a7 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -362,8 +362,9 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
     datalen = stab.st_size;
 
     if (stab.st_size && data_r) {
-        data = malloc(datalen);
+        data = malloc(((size_t)datalen)+1);
         if (!data) goto xe;
+        data[datalen] = 0;  /* make sure buffer null-terminated */
 
         rs = fread(data, 1, datalen, f);
         if (rs != datalen) {
-- 
1.8.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[Wei Liu]

On Wed, Jun 27, 2018 at 09:13:11AM +0000, Robin Lee wrote:
> On XenServer 7.1.1, we start a vm with XAPI but attach a block device with xl.
> We create an empty json config for the vm with the content "{}\n" and then
> run 'xl block-attach':
> 
>   #  xl block-attach 1 phy:/dev/loop0 xvdz w
>   libxl: error: libxl_json.c:950:libxl__json_parse: yajl error: parse error: trailing garbage
>                                       {} K]
>                        (right here) ------^
> 
>   libxl: error: libxl_json.c:1053:libxl__object_from_json: unable to generate libxl__json_object from JSON representation of libxl_domain_config.
>   libxl: error: libxl.c:1995:device_addrm_aocomplete: unable to add device
>   libxl_device_disk_add failed.
> 
> After investigation, we found the buffer returned from libxl_read_file_contents
> is not null-terminated. But later in libxl__object_from_json, the buffer is expected to
> be null-terminated. So parsing may exceeded the end of file and get in to uninisialized
> momery area.
> 
> Signed-off-by: Robin Lee <robinlee.sysu@gmail.com>

I can't seem to be able to reproduce this in upstream xen. Which version
of Xen does XenServer 7.1.1 have? You can get that from the output of
`xl info` -- look for xen_{major, minor, extra}.

BTW if you're using XenServer you probably should use XAPI to manipulate
guests instead.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[Robin Lee]

On Wed, Jun 27, 2018 at 6:58 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Wed, Jun 27, 2018 at 09:13:11AM +0000, Robin Lee wrote:
>> On XenServer 7.1.1, we start a vm with XAPI but attach a block device with xl.
>> We create an empty json config for the vm with the content "{}\n" and then
>> run 'xl block-attach':
>>
>>   #  xl block-attach 1 phy:/dev/loop0 xvdz w
>>   libxl: error: libxl_json.c:950:libxl__json_parse: yajl error: parse error: trailing garbage
>>                                       {} K]
>>                        (right here) ------^
>>
>>   libxl: error: libxl_json.c:1053:libxl__object_from_json: unable to generate libxl__json_object from JSON representation of libxl_domain_config.
>>   libxl: error: libxl.c:1995:device_addrm_aocomplete: unable to add device
>>   libxl_device_disk_add failed.
>>
>> After investigation, we found the buffer returned from libxl_read_file_contents
>> is not null-terminated. But later in libxl__object_from_json, the buffer is expected to
>> be null-terminated. So parsing may exceeded the end of file and get in to uninisialized
>> momery area.
>>
>> Signed-off-by: Robin Lee <robinlee.sysu@gmail.com>
>
> I can't seem to be able to reproduce this in upstream xen. Which version
> of Xen does XenServer 7.1.1 have? You can get that from the output of
> `xl info` -- look for xen_{major, minor, extra}.
I also met a strange case. We didn't see this problem with Xen 4.7.1
that released with
XenServer 7.1.1. But since a recently hotfix from XenServer that upgraded Xen to
4.7.4, this problem then shows up.

The version of yajl (yajl-2.0.4-4.el7.x86_64)  never changed.
>
> BTW if you're using XenServer you probably should use XAPI to manipulate
> guests instead.
>
> Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[Andrew Cooper]

On 27/06/18 12:08, Robin Lee wrote:
> On Wed, Jun 27, 2018 at 6:58 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>> On Wed, Jun 27, 2018 at 09:13:11AM +0000, Robin Lee wrote:
>>> On XenServer 7.1.1, we start a vm with XAPI but attach a block device with xl.
>>> We create an empty json config for the vm with the content "{}\n" and then
>>> run 'xl block-attach':
>>>
>>>   #  xl block-attach 1 phy:/dev/loop0 xvdz w
>>>   libxl: error: libxl_json.c:950:libxl__json_parse: yajl error: parse error: trailing garbage
>>>                                       {} K]
>>>                        (right here) ------^
>>>
>>>   libxl: error: libxl_json.c:1053:libxl__object_from_json: unable to generate libxl__json_object from JSON representation of libxl_domain_config.
>>>   libxl: error: libxl.c:1995:device_addrm_aocomplete: unable to add device
>>>   libxl_device_disk_add failed.
>>>
>>> After investigation, we found the buffer returned from libxl_read_file_contents
>>> is not null-terminated. But later in libxl__object_from_json, the buffer is expected to
>>> be null-terminated. So parsing may exceeded the end of file and get in to uninisialized
>>> momery area.
>>>
>>> Signed-off-by: Robin Lee <robinlee.sysu@gmail.com>
>> I can't seem to be able to reproduce this in upstream xen. Which version
>> of Xen does XenServer 7.1.1 have? You can get that from the output of
>> `xl info` -- look for xen_{major, minor, extra}.
> I also met a strange case. We didn't see this problem with Xen 4.7.1
> that released with
> XenServer 7.1.1. But since a recently hotfix from XenServer that upgraded Xen to
> 4.7.4, this problem then shows up.

A later hotfix even than that will bump to 4.7.5.  (This is the
Spectre/Meltdown mitigations.)

XenServer has no interesting patches to libxl/xl, so this looks like it
may have been a regression which was backported into the stable trees.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[Wei Liu]

On Wed, Jun 27, 2018 at 07:08:02PM +0800, Robin Lee wrote:
> On Wed, Jun 27, 2018 at 6:58 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Wed, Jun 27, 2018 at 09:13:11AM +0000, Robin Lee wrote:
> >> On XenServer 7.1.1, we start a vm with XAPI but attach a block device with xl.
> >> We create an empty json config for the vm with the content "{}\n" and then
> >> run 'xl block-attach':
> >>
> >>   #  xl block-attach 1 phy:/dev/loop0 xvdz w
> >>   libxl: error: libxl_json.c:950:libxl__json_parse: yajl error: parse error: trailing garbage
> >>                                       {} K]
> >>                        (right here) ------^
> >>
> >>   libxl: error: libxl_json.c:1053:libxl__object_from_json: unable to generate libxl__json_object from JSON representation of libxl_domain_config.
> >>   libxl: error: libxl.c:1995:device_addrm_aocomplete: unable to add device
> >>   libxl_device_disk_add failed.
> >>
> >> After investigation, we found the buffer returned from libxl_read_file_contents
> >> is not null-terminated. But later in libxl__object_from_json, the buffer is expected to
> >> be null-terminated. So parsing may exceeded the end of file and get in to uninisialized
> >> momery area.
> >>
> >> Signed-off-by: Robin Lee <robinlee.sysu@gmail.com>
> >
> > I can't seem to be able to reproduce this in upstream xen. Which version
> > of Xen does XenServer 7.1.1 have? You can get that from the output of
> > `xl info` -- look for xen_{major, minor, extra}.
> I also met a strange case. We didn't see this problem with Xen 4.7.1
> that released with
> XenServer 7.1.1. But since a recently hotfix from XenServer that upgraded Xen to
> 4.7.4, this problem then shows up.
> 
> The version of yajl (yajl-2.0.4-4.el7.x86_64)  never changed.

As far as I can tell, the stored json file already contains trailing 0,
even in 4.7.4. There is nothing interesting between 4.7.1 and 4.7.4 in
that area of code.

I'm afraid I can't take in your patch before we understand why the code
doesn't function as expected. The problem is likely to be somewhere
else.

You can inspect the files under /var/lib/xen (or the location configured
by XenServer). The names are like userdata-d.X.$UUID.libxl-json.

One theory is the upgrade left some stale libraries which your xl then
used. Use ldd `which xl` to see which libraries it used. And make sure
it uses the latest ones that come with the upgrade.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[Robin Lee]

On Wed, Jun 27, 2018 at 7:24 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Wed, Jun 27, 2018 at 07:08:02PM +0800, Robin Lee wrote:
>> On Wed, Jun 27, 2018 at 6:58 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>> > On Wed, Jun 27, 2018 at 09:13:11AM +0000, Robin Lee wrote:
>> >> On XenServer 7.1.1, we start a vm with XAPI but attach a block device with xl.
>> >> We create an empty json config for the vm with the content "{}\n" and then
>> >> run 'xl block-attach':
>> >>
>> >>   #  xl block-attach 1 phy:/dev/loop0 xvdz w
>> >>   libxl: error: libxl_json.c:950:libxl__json_parse: yajl error: parse error: trailing garbage
>> >>                                       {} K]
>> >>                        (right here) ------^
>> >>
>> >>   libxl: error: libxl_json.c:1053:libxl__object_from_json: unable to generate libxl__json_object from JSON representation of libxl_domain_config.
>> >>   libxl: error: libxl.c:1995:device_addrm_aocomplete: unable to add device
>> >>   libxl_device_disk_add failed.
>> >>
>> >> After investigation, we found the buffer returned from libxl_read_file_contents
>> >> is not null-terminated. But later in libxl__object_from_json, the buffer is expected to
>> >> be null-terminated. So parsing may exceeded the end of file and get in to uninisialized
>> >> momery area.
>> >>
>> >> Signed-off-by: Robin Lee <robinlee.sysu@gmail.com>
>> >
>> > I can't seem to be able to reproduce this in upstream xen. Which version
>> > of Xen does XenServer 7.1.1 have? You can get that from the output of
>> > `xl info` -- look for xen_{major, minor, extra}.
>> I also met a strange case. We didn't see this problem with Xen 4.7.1
>> that released with
>> XenServer 7.1.1. But since a recently hotfix from XenServer that upgraded Xen to
>> 4.7.4, this problem then shows up.
>>
>> The version of yajl (yajl-2.0.4-4.el7.x86_64)  never changed.
>
> As far as I can tell, the stored json file already contains trailing 0,
> even in 4.7.4. There is nothing interesting between 4.7.1 and 4.7.4 in
> that area of code.
In my situation, the json file is created with external program and contains
just "{}\n" and not trailing 0.
>
> I'm afraid I can't take in your patch before we understand why the code
> doesn't function as expected. The problem is likely to be somewhere
> else.
>
> You can inspect the files under /var/lib/xen (or the location configured
> by XenServer). The names are like userdata-d.X.$UUID.libxl-json.
>
> One theory is the upgrade left some stale libraries which your xl then
> used. Use ldd `which xl` to see which libraries it used. And make sure
> it uses the latest ones that come with the upgrade.
>
> Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[Wei Liu]

On Wed, Jun 27, 2018 at 07:37:42PM +0800, Robin Lee wrote:
> On Wed, Jun 27, 2018 at 7:24 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Wed, Jun 27, 2018 at 07:08:02PM +0800, Robin Lee wrote:
> >> On Wed, Jun 27, 2018 at 6:58 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> >> > On Wed, Jun 27, 2018 at 09:13:11AM +0000, Robin Lee wrote:
> >> >> On XenServer 7.1.1, we start a vm with XAPI but attach a block device with xl.
> >> >> We create an empty json config for the vm with the content "{}\n" and then
> >> >> run 'xl block-attach':
> >> >>
> >> >>   #  xl block-attach 1 phy:/dev/loop0 xvdz w
> >> >>   libxl: error: libxl_json.c:950:libxl__json_parse: yajl error: parse error: trailing garbage
> >> >>                                       {} K]
> >> >>                        (right here) ------^
> >> >>
> >> >>   libxl: error: libxl_json.c:1053:libxl__object_from_json: unable to generate libxl__json_object from JSON representation of libxl_domain_config.
> >> >>   libxl: error: libxl.c:1995:device_addrm_aocomplete: unable to add device
> >> >>   libxl_device_disk_add failed.
> >> >>
> >> >> After investigation, we found the buffer returned from libxl_read_file_contents
> >> >> is not null-terminated. But later in libxl__object_from_json, the buffer is expected to
> >> >> be null-terminated. So parsing may exceeded the end of file and get in to uninisialized
> >> >> momery area.
> >> >>
> >> >> Signed-off-by: Robin Lee <robinlee.sysu@gmail.com>
> >> >
> >> > I can't seem to be able to reproduce this in upstream xen. Which version
> >> > of Xen does XenServer 7.1.1 have? You can get that from the output of
> >> > `xl info` -- look for xen_{major, minor, extra}.
> >> I also met a strange case. We didn't see this problem with Xen 4.7.1
> >> that released with
> >> XenServer 7.1.1. But since a recently hotfix from XenServer that upgraded Xen to
> >> 4.7.4, this problem then shows up.
> >>
> >> The version of yajl (yajl-2.0.4-4.el7.x86_64)  never changed.
> >
> > As far as I can tell, the stored json file already contains trailing 0,
> > even in 4.7.4. There is nothing interesting between 4.7.1 and 4.7.4 in
> > that area of code.
> In my situation, the json file is created with external program and contains
> just "{}\n" and not trailing 0.

Alright. In that case please append 0 to the file you created.

The json files are considered to be internal to libxl.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[Robin Lee]

On Wed, Jun 27, 2018 at 8:29 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Wed, Jun 27, 2018 at 07:37:42PM +0800, Robin Lee wrote:
>> On Wed, Jun 27, 2018 at 7:24 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>> > On Wed, Jun 27, 2018 at 07:08:02PM +0800, Robin Lee wrote:
>> >> On Wed, Jun 27, 2018 at 6:58 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>> >> > On Wed, Jun 27, 2018 at 09:13:11AM +0000, Robin Lee wrote:
>> >> >> On XenServer 7.1.1, we start a vm with XAPI but attach a block device with xl.
>> >> >> We create an empty json config for the vm with the content "{}\n" and then
>> >> >> run 'xl block-attach':
>> >> >>
>> >> >>   #  xl block-attach 1 phy:/dev/loop0 xvdz w
>> >> >>   libxl: error: libxl_json.c:950:libxl__json_parse: yajl error: parse error: trailing garbage
>> >> >>                                       {} K]
>> >> >>                        (right here) ------^
>> >> >>
>> >> >>   libxl: error: libxl_json.c:1053:libxl__object_from_json: unable to generate libxl__json_object from JSON representation of libxl_domain_config.
>> >> >>   libxl: error: libxl.c:1995:device_addrm_aocomplete: unable to add device
>> >> >>   libxl_device_disk_add failed.
>> >> >>
>> >> >> After investigation, we found the buffer returned from libxl_read_file_contents
>> >> >> is not null-terminated. But later in libxl__object_from_json, the buffer is expected to
>> >> >> be null-terminated. So parsing may exceeded the end of file and get in to uninisialized
>> >> >> momery area.
>> >> >>
>> >> >> Signed-off-by: Robin Lee <robinlee.sysu@gmail.com>
>> >> >
>> >> > I can't seem to be able to reproduce this in upstream xen. Which version
>> >> > of Xen does XenServer 7.1.1 have? You can get that from the output of
>> >> > `xl info` -- look for xen_{major, minor, extra}.
>> >> I also met a strange case. We didn't see this problem with Xen 4.7.1
>> >> that released with
>> >> XenServer 7.1.1. But since a recently hotfix from XenServer that upgraded Xen to
>> >> 4.7.4, this problem then shows up.
>> >>
>> >> The version of yajl (yajl-2.0.4-4.el7.x86_64)  never changed.
>> >
>> > As far as I can tell, the stored json file already contains trailing 0,
>> > even in 4.7.4. There is nothing interesting between 4.7.1 and 4.7.4 in
>> > that area of code.
>> In my situation, the json file is created with external program and contains
>> just "{}\n" and not trailing 0.
>
> Alright. In that case please append 0 to the file you created.
>
> The json files are considered to be internal to libxl.
OK. I can conform that json file generated by xl contains a trailing 0.
But that seems not a common design to rely on the trailing 0 inside a text file.
>
> Wei.
-robin

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[Wei Liu]

On Thu, Jun 28, 2018 at 10:27:33AM +0800, Robin Lee wrote:
> On Wed, Jun 27, 2018 at 8:29 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> > On Wed, Jun 27, 2018 at 07:37:42PM +0800, Robin Lee wrote:
> >> On Wed, Jun 27, 2018 at 7:24 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> >> > On Wed, Jun 27, 2018 at 07:08:02PM +0800, Robin Lee wrote:
> >> >> On Wed, Jun 27, 2018 at 6:58 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> >> >> > On Wed, Jun 27, 2018 at 09:13:11AM +0000, Robin Lee wrote:
> >> >> >> On XenServer 7.1.1, we start a vm with XAPI but attach a block device with xl.
> >> >> >> We create an empty json config for the vm with the content "{}\n" and then
> >> >> >> run 'xl block-attach':
> >> >> >>
> >> >> >>   #  xl block-attach 1 phy:/dev/loop0 xvdz w
> >> >> >>   libxl: error: libxl_json.c:950:libxl__json_parse: yajl error: parse error: trailing garbage
> >> >> >>                                       {} K]
> >> >> >>                        (right here) ------^
> >> >> >>
> >> >> >>   libxl: error: libxl_json.c:1053:libxl__object_from_json: unable to generate libxl__json_object from JSON representation of libxl_domain_config.
> >> >> >>   libxl: error: libxl.c:1995:device_addrm_aocomplete: unable to add device
> >> >> >>   libxl_device_disk_add failed.
> >> >> >>
> >> >> >> After investigation, we found the buffer returned from libxl_read_file_contents
> >> >> >> is not null-terminated. But later in libxl__object_from_json, the buffer is expected to
> >> >> >> be null-terminated. So parsing may exceeded the end of file and get in to uninisialized
> >> >> >> momery area.
> >> >> >>
> >> >> >> Signed-off-by: Robin Lee <robinlee.sysu@gmail.com>
> >> >> >
> >> >> > I can't seem to be able to reproduce this in upstream xen. Which version
> >> >> > of Xen does XenServer 7.1.1 have? You can get that from the output of
> >> >> > `xl info` -- look for xen_{major, minor, extra}.
> >> >> I also met a strange case. We didn't see this problem with Xen 4.7.1
> >> >> that released with
> >> >> XenServer 7.1.1. But since a recently hotfix from XenServer that upgraded Xen to
> >> >> 4.7.4, this problem then shows up.
> >> >>
> >> >> The version of yajl (yajl-2.0.4-4.el7.x86_64)  never changed.
> >> >
> >> > As far as I can tell, the stored json file already contains trailing 0,
> >> > even in 4.7.4. There is nothing interesting between 4.7.1 and 4.7.4 in
> >> > that area of code.
> >> In my situation, the json file is created with external program and contains
> >> just "{}\n" and not trailing 0.
> >
> > Alright. In that case please append 0 to the file you created.
> >
> > The json files are considered to be internal to libxl.
> OK. I can conform that json file generated by xl contains a trailing 0.
> But that seems not a common design to rely on the trailing 0 inside a text file.

I know it is confusing but you probably shouldn't treat libxl-json file
as a text file. :-)

This also means what you previous did worked by chance -- it probably
worked because there happened to be 0's after the loaded buffer.

Wei.

> >
> > Wei.
> -robin

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[Andrew Cooper]

On 28/06/18 11:17, Wei Liu wrote:
> On Thu, Jun 28, 2018 at 10:27:33AM +0800, Robin Lee wrote:
>> On Wed, Jun 27, 2018 at 8:29 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>>> On Wed, Jun 27, 2018 at 07:37:42PM +0800, Robin Lee wrote:
>>>> On Wed, Jun 27, 2018 at 7:24 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>>>>> On Wed, Jun 27, 2018 at 07:08:02PM +0800, Robin Lee wrote:
>>>>>> On Wed, Jun 27, 2018 at 6:58 PM, Wei Liu <wei.liu2@citrix.com> wrote:
>>>>>>> On Wed, Jun 27, 2018 at 09:13:11AM +0000, Robin Lee wrote:
>>>>>>>> On XenServer 7.1.1, we start a vm with XAPI but attach a block device with xl.
>>>>>>>> We create an empty json config for the vm with the content "{}\n" and then
>>>>>>>> run 'xl block-attach':
>>>>>>>>
>>>>>>>>   #  xl block-attach 1 phy:/dev/loop0 xvdz w
>>>>>>>>   libxl: error: libxl_json.c:950:libxl__json_parse: yajl error: parse error: trailing garbage
>>>>>>>>                                       {} K]
>>>>>>>>                        (right here) ------^
>>>>>>>>
>>>>>>>>   libxl: error: libxl_json.c:1053:libxl__object_from_json: unable to generate libxl__json_object from JSON representation of libxl_domain_config.
>>>>>>>>   libxl: error: libxl.c:1995:device_addrm_aocomplete: unable to add device
>>>>>>>>   libxl_device_disk_add failed.
>>>>>>>>
>>>>>>>> After investigation, we found the buffer returned from libxl_read_file_contents
>>>>>>>> is not null-terminated. But later in libxl__object_from_json, the buffer is expected to
>>>>>>>> be null-terminated. So parsing may exceeded the end of file and get in to uninisialized
>>>>>>>> momery area.
>>>>>>>>
>>>>>>>> Signed-off-by: Robin Lee <robinlee.sysu@gmail.com>
>>>>>>> I can't seem to be able to reproduce this in upstream xen. Which version
>>>>>>> of Xen does XenServer 7.1.1 have? You can get that from the output of
>>>>>>> `xl info` -- look for xen_{major, minor, extra}.
>>>>>> I also met a strange case. We didn't see this problem with Xen 4.7.1
>>>>>> that released with
>>>>>> XenServer 7.1.1. But since a recently hotfix from XenServer that upgraded Xen to
>>>>>> 4.7.4, this problem then shows up.
>>>>>>
>>>>>> The version of yajl (yajl-2.0.4-4.el7.x86_64)  never changed.
>>>>> As far as I can tell, the stored json file already contains trailing 0,
>>>>> even in 4.7.4. There is nothing interesting between 4.7.1 and 4.7.4 in
>>>>> that area of code.
>>>> In my situation, the json file is created with external program and contains
>>>> just "{}\n" and not trailing 0.
>>> Alright. In that case please append 0 to the file you created.
>>>
>>> The json files are considered to be internal to libxl.
>> OK. I can conform that json file generated by xl contains a trailing 0.
>> But that seems not a common design to rely on the trailing 0 inside a text file.
> I know it is confusing but you probably shouldn't treat libxl-json file
> as a text file. :-)
>
> This also means what you previous did worked by chance -- it probably
> worked because there happened to be 0's after the loaded buffer.

I know libxl isn't hardened at all against bad json, but it is
unreasonable for libxl to depend on there being a \0 at the end of a
json file (which is a text file to literally everyone else and all their
tools).

What is the current behaviour in staging?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents [and 3 more messages]

[Ian Jackson]

Taking things in order from most salient to least salient:

Robin Lee writes ("Re: [Xen-devel] [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents"):
> In my situation, the json file is created with external program and
> contains just "{}\n" and not trailing 0.

I just want to be sure I understand correctly.  You are creating the
libxl domain config userdata json yourself, and stuffing it into
/var/lib/xen ?  That file is internal to libxl and doing that is,
obviously, not supported.

I think, though, that what "not supported" means is that the format
and storage location and so on may change, and that you are on your
own if it does.

However, I don't think it means that if you discover bugs or
infelicities, we shouldn't fix them.  So:

Wei Liu writes ("Re: [Xen-devel] [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents"):
> Alright. In that case please append 0 to the file you created.

I don't really agree that this is the right answer.  Thanks, to Robin,
for bringing this to our attention.

It is clearly bizarre that this file, which in the design is supposed
to be json, (i) happens to contain a trailing nul (ii) which is
necessary for correct operation.

We cannot fix (i) without invalidating old files, which we don't want
to do.  But we should fix (ii).

I am inclined to think that something along the lines of the original
patch is the way to do that.  Although, this extra nul byte should be
documented in the API comment then.

Also, we should identify where the additional nul byte is coming
from.  While we can't just delete that, we should put a comment in
saying that the off-by-one error is deliberate.

Wei Liu writes ("Re: [Xen-devel] [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents"):
> BTW if you're using XenServer you probably should use XAPI to manipulate
> guests instead.

I don't entirely agree with this either.  Obviously mixing and
matching like this is not a supported configuration, in the sense that
you don't get any stability guarantees.

But in practice it is likely to work (provided XAPI does not get
confused) and it seems like part of a reasonable transition strategy
for a complicated system to make more use of libxl.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents

[Wei Liu]

On Thu, Jun 28, 2018 at 11:21:35AM +0100, Andrew Cooper wrote:
> On 28/06/18 11:17, Wei Liu wrote:
> > On Thu, Jun 28, 2018 at 10:27:33AM +0800, Robin Lee wrote:
> >> On Wed, Jun 27, 2018 at 8:29 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> >>> On Wed, Jun 27, 2018 at 07:37:42PM +0800, Robin Lee wrote:
> >>>> On Wed, Jun 27, 2018 at 7:24 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> >>>>> On Wed, Jun 27, 2018 at 07:08:02PM +0800, Robin Lee wrote:
> >>>>>> On Wed, Jun 27, 2018 at 6:58 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> >>>>>>> On Wed, Jun 27, 2018 at 09:13:11AM +0000, Robin Lee wrote:
> >>>>>>>> On XenServer 7.1.1, we start a vm with XAPI but attach a block device with xl.
> >>>>>>>> We create an empty json config for the vm with the content "{}\n" and then
> >>>>>>>> run 'xl block-attach':
> >>>>>>>>
> >>>>>>>>   #  xl block-attach 1 phy:/dev/loop0 xvdz w
> >>>>>>>>   libxl: error: libxl_json.c:950:libxl__json_parse: yajl error: parse error: trailing garbage
> >>>>>>>>                                       {} K]
> >>>>>>>>                        (right here) ------^
> >>>>>>>>
> >>>>>>>>   libxl: error: libxl_json.c:1053:libxl__object_from_json: unable to generate libxl__json_object from JSON representation of libxl_domain_config.
> >>>>>>>>   libxl: error: libxl.c:1995:device_addrm_aocomplete: unable to add device
> >>>>>>>>   libxl_device_disk_add failed.
> >>>>>>>>
> >>>>>>>> After investigation, we found the buffer returned from libxl_read_file_contents
> >>>>>>>> is not null-terminated. But later in libxl__object_from_json, the buffer is expected to
> >>>>>>>> be null-terminated. So parsing may exceeded the end of file and get in to uninisialized
> >>>>>>>> momery area.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Robin Lee <robinlee.sysu@gmail.com>
> >>>>>>> I can't seem to be able to reproduce this in upstream xen. Which version
> >>>>>>> of Xen does XenServer 7.1.1 have? You can get that from the output of
> >>>>>>> `xl info` -- look for xen_{major, minor, extra}.
> >>>>>> I also met a strange case. We didn't see this problem with Xen 4.7.1
> >>>>>> that released with
> >>>>>> XenServer 7.1.1. But since a recently hotfix from XenServer that upgraded Xen to
> >>>>>> 4.7.4, this problem then shows up.
> >>>>>>
> >>>>>> The version of yajl (yajl-2.0.4-4.el7.x86_64)  never changed.
> >>>>> As far as I can tell, the stored json file already contains trailing 0,
> >>>>> even in 4.7.4. There is nothing interesting between 4.7.1 and 4.7.4 in
> >>>>> that area of code.
> >>>> In my situation, the json file is created with external program and contains
> >>>> just "{}\n" and not trailing 0.
> >>> Alright. In that case please append 0 to the file you created.
> >>>
> >>> The json files are considered to be internal to libxl.
> >> OK. I can conform that json file generated by xl contains a trailing 0.
> >> But that seems not a common design to rely on the trailing 0 inside a text file.
> > I know it is confusing but you probably shouldn't treat libxl-json file
> > as a text file. :-)
> >
> > This also means what you previous did worked by chance -- it probably
> > worked because there happened to be 0's after the loaded buffer.
> 
> I know libxl isn't hardened at all against bad json, but it is
> unreasonable for libxl to depend on there being a \0 at the end of a
> json file (which is a text file to literally everyone else and all their
> tools).

The file in question is an internal state file which is private to the
library, which is assumed to have a good form.  It is a different story
if arbitrary json is accepted as input into the library.

I wouldn't mind changing the internal format, but that doesn't change
the fact that the file is private and is not supposed to be changed
directly by users.

> 
> What is the current behaviour in staging?

The same. The code hasn't changed since 4.7.4.

Wei.

> 
> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents [and 3 more messages]

[Wei Liu]

On Thu, Jun 28, 2018 at 11:24:42AM +0100, Ian Jackson wrote:
> Taking things in order from most salient to least salient:
> 
> Robin Lee writes ("Re: [Xen-devel] [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents"):
> > In my situation, the json file is created with external program and
> > contains just "{}\n" and not trailing 0.
> 
> I just want to be sure I understand correctly.  You are creating the
> libxl domain config userdata json yourself, and stuffing it into
> /var/lib/xen ?  That file is internal to libxl and doing that is,
> obviously, not supported.
> 
> I think, though, that what "not supported" means is that the format
> and storage location and so on may change, and that you are on your
> own if it does.
> 
> However, I don't think it means that if you discover bugs or
> infelicities, we shouldn't fix them.  So:
> 
> Wei Liu writes ("Re: [Xen-devel] [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents"):
> > Alright. In that case please append 0 to the file you created.
> 
> I don't really agree that this is the right answer.  Thanks, to Robin,
> for bringing this to our attention.
> 
> It is clearly bizarre that this file, which in the design is supposed
> to be json, (i) happens to contain a trailing nul (ii) which is
> necessary for correct operation.
> 
> We cannot fix (i) without invalidating old files, which we don't want
> to do.  But we should fix (ii).
> 

We can safely remove the trailing nul if libxl API makes sure a buffer
is nul-terminated. Old files will still work with new library. New files
won't work with old library -- but we don't support that.

> I am inclined to think that something along the lines of the original
> patch is the way to do that.  Although, this extra nul byte should be
> documented in the API comment then.
> 
> Also, we should identify where the additional nul byte is coming
> from.  While we can't just delete that, we should put a comment in
> saying that the off-by-one error is deliberate.

What off-by-one error?

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents [and 3 more messages]

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents [and 3 more messages]"):
> We can safely remove the trailing nul if libxl API makes sure a buffer
> is nul-terminated. Old files will still work with new library. New files
> won't work with old library -- but we don't support that.

I guess.

> > I am inclined to think that something along the lines of the original
> > patch is the way to do that.  Although, this extra nul byte should be
> > documented in the API comment then.
> > 
> > Also, we should identify where the additional nul byte is coming
> > from.  While we can't just delete that, we should put a comment in
> > saying that the off-by-one error is deliberate.
> 
> What off-by-one error?

The one that writes a nul to the file.  I can't believe it was
deliberate.  I haven't looked to find where it comes from but if the
code where it comes from doesn't look like an off-by-one error then it
is too obtuse :-).

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents [and 3 more messages]

[Wei Liu]

On Thu, Jun 28, 2018 at 12:35:38PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [Xen-devel] [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents [and 3 more messages]"):
> > We can safely remove the trailing nul if libxl API makes sure a buffer
> > is nul-terminated. Old files will still work with new library. New files
> > won't work with old library -- but we don't support that.
> 
> I guess.
> 
> > > I am inclined to think that something along the lines of the original
> > > patch is the way to do that.  Although, this extra nul byte should be
> > > documented in the API comment then.
> > > 
> > > Also, we should identify where the additional nul byte is coming
> > > from.  While we can't just delete that, we should put a comment in
> > > saying that the off-by-one error is deliberate.
> > 
> > What off-by-one error?
> 
> The one that writes a nul to the file.  I can't believe it was
> deliberate.  I haven't looked to find where it comes from but if the
> code where it comes from doesn't look like an off-by-one error then it
> is too obtuse :-).

It is deliberate. See libxl_internal.h:libxl__set_domain_configuration.

At the time I wrote the code I didn't want to change the read/write
function APIs. The file was / is considered internal anyway.

Wei.

> 
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents [and 3 more messages]

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH] libxl: make sure buffer is null-terminated in libxl_read_file_contents [and 3 more messages]"):
> On Thu, Jun 28, 2018 at 12:35:38PM +0100, Ian Jackson wrote:
> > The one that writes a nul to the file.  I can't believe it was
> > deliberate.  I haven't looked to find where it comes from but if the
> > code where it comes from doesn't look like an off-by-one error then it
> > is too obtuse :-).
> 
> It is deliberate. See libxl_internal.h:libxl__set_domain_configuration.

The header file says nothing about this.  It seems to imply that the
file is json, which, if it contains a nul, it isn't.  I do see that
the implementation does make the off-by-one obviously deliberate.

> At the time I wrote the code I didn't want to change the read/write
> function APIs. The file was / is considered internal anyway.

I see.

Well, I think the read/write API is nicer if it includes a trailing
nul.  ISTR some other caller of those functions wanting a nul too.
And it would be better if the file were actual json, even if many json
actual pretty printers will tolerate the nul.

So, my preference would be for the original patch to be resubmitted
with a summary of this thread in the commit message.

Maybe in some future release, an additional patch to remove the nul,
although I care about that a lot less.  I don't see any reason to
remove the nul proactively given that it might break people doing
unsupported things like going backwards with their libxl version.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel