[daisy][PATCH 0/1] systemd: do not use alloca() function in case of uclibc

[daisy][PATCH 0/1] systemd: do not use alloca() function in case of uclibc

[Chen Qi]

The following changes since commit af347d3298e15552d502d5b2ce497bbda9705bc7:

  binutils: Fix building nativesdk binutils with gcc 4.9 (2014-05-29 13:42:13 +0100)

are available in the git repository at:

  git://git.openembedded.org/openembedded-core-contrib ChenQi/daisy-systemd-alloca
  http://cgit.openembedded.org/cgit.cgi/openembedded-core-contrib/log/?h=ChenQi/daisy-systemd-alloca

Chen Qi (1):
  systemd: do not use alloca() function in case of uclibc

 .../0001-journal-file.c-do-not-use-alloca.patch    |   54 ++++++++++++++++++++
 meta/recipes-core/systemd/systemd_211.bb           |    1 +
 2 files changed, 55 insertions(+)
 create mode 100644 meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch

-- 
1.7.9.5

[daisy][PATCH 1/1] systemd: do not use alloca() function in case of uclibc

[Chen Qi]

The alloca() function allocates space in the stack frame of the caller,
so using alloca(new_size - old_size) would possibly crash the stack,
causing a segment fault error.

This patch fixes the above problem by avoiding using this function in
journal-file.c.

[YOCTO #6201]

Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
---
 .../0001-journal-file.c-do-not-use-alloca.patch    |   54 ++++++++++++++++++++
 meta/recipes-core/systemd/systemd_211.bb           |    1 +
 2 files changed, 55 insertions(+)
 create mode 100644 meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch

diff --git a/meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch b/meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch
new file mode 100644
index 0000000..a638d58
--- /dev/null
+++ b/meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch
@@ -0,0 +1,54 @@
+Upstream-Status: Inappropriate [oe specific]
+
+journal-file.c: do not use alloca
+
+Using alloca(new_size - old_size) would possibly cause a segment fault
+error. Note that the alloca function allocates space in the stack frame
+of the caller. So it's possible that the allocated size exceeds the stack
+size limit.
+
+There's actually no need to allocate stack here, we only need to write some
+bytes to the block to make sure the needed block space is available, just like
+eglibc does in posix_fallocate.
+
+Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
+
+---
+ src/journal/journal-file.c |   21 +++++++++------------
+ 1 file changed, 9 insertions(+), 12 deletions(-)
+
+diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c
+index cef8bba..e364298 100644
+--- a/src/journal/journal-file.c
++++ b/src/journal/journal-file.c
+@@ -371,18 +371,15 @@ static int journal_file_allocate(JournalFile *f, uint64_t offset, uint64_t size)
+         if (r != 0)
+                 return -r;
+ #else
+-       /* Use good old method to write zeros into the journal file
+-          perhaps very inefficient yet working. */
+-       if(new_size > old_size) {
+-               char *buf = alloca(new_size - old_size);
+-               off_t oldpos = lseek(f->fd, 0, SEEK_CUR);
+-               bzero(buf, new_size - old_size);
+-               lseek(f->fd, old_size, SEEK_SET);
+-               r = write(f->fd, buf, new_size - old_size);
+-               lseek(f->fd, oldpos, SEEK_SET);
+-       }
+-       if (r < 0)
+-               return -errno;
++        /* Write something every 512 bytes to make sure the block is allocated */
++        uint64_t len = new_size - old_size;
++        uint64_t offset = old_size;
++        for (offset += (len-1) % 512; len > 0; offset += 512) {
++                len -= 512;
++                if (pwrite(f->fd, "", 1, offset) != 1)
++                        return -errno;
++        }
++
+ #endif /* HAVE_POSIX_FALLOCATE */
+ 
+         if (fstat(f->fd, &f->last_stat) < 0)
+-- 
+1.7.9.5
+
diff --git a/meta/recipes-core/systemd/systemd_211.bb b/meta/recipes-core/systemd/systemd_211.bb
index 44b1965..3eb3778 100644
--- a/meta/recipes-core/systemd/systemd_211.bb
+++ b/meta/recipes-core/systemd/systemd_211.bb
@@ -32,6 +32,7 @@ SRC_URI = "git://anongit.freedesktop.org/systemd/systemd;branch=master;protocol=
            file://uclibc-sysinfo_h.patch \
            file://uclibc-get-physmem.patch \
            file://sd-bus-don-t-use-assert_return-to-check-for-disconne.patch \
+           file://0001-journal-file.c-do-not-use-alloca.patch \
            \
            file://touchscreen.rules \
            file://00-create-volatile.conf \
-- 
1.7.9.5

Re: [daisy][PATCH 1/1] systemd: do not use alloca() function in case of uclibc

[Richard Purdie]

On Tue, 2014-06-03 at 15:42 +0800, Chen Qi wrote:
> The alloca() function allocates space in the stack frame of the caller,
> so using alloca(new_size - old_size) would possibly crash the stack,
> causing a segment fault error.
> 
> This patch fixes the above problem by avoiding using this function in
> journal-file.c.
> 
> [YOCTO #6201]
> 
> Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
> ---
>  .../0001-journal-file.c-do-not-use-alloca.patch    |   54 ++++++++++++++++++++
>  meta/recipes-core/systemd/systemd_211.bb           |    1 +
>  2 files changed, 55 insertions(+)
>  create mode 100644 meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch
> 
> diff --git a/meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch b/meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch
> new file mode 100644
> index 0000000..a638d58
> --- /dev/null
> +++ b/meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch
> @@ -0,0 +1,54 @@
> +Upstream-Status: Inappropriate [oe specific]

From the description, this sounds like an allocation error which can
happen *anywhere* and is a problem that should be addressed upstream.

This Upstream-Status field is therefore completely bogus. Its not
inappropriate or oe specific. If you still believe it is, I'd like to
hear more explanation.

The abuses of this field are starting to really annoy me since this
keeps happening.

Cheers,

Richard

Re: [daisy][PATCH 1/1] systemd: do not use alloca() function in case of uclibc

[ChenQi]

On 06/03/2014 05:06 PM, Richard Purdie wrote:
> On Tue, 2014-06-03 at 15:42 +0800, Chen Qi wrote:
>> The alloca() function allocates space in the stack frame of the caller,
>> so using alloca(new_size - old_size) would possibly crash the stack,
>> causing a segment fault error.
>>
>> This patch fixes the above problem by avoiding using this function in
>> journal-file.c.
>>
>> [YOCTO #6201]
>>
>> Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
>> ---
>>   .../0001-journal-file.c-do-not-use-alloca.patch    |   54 ++++++++++++++++++++
>>   meta/recipes-core/systemd/systemd_211.bb           |    1 +
>>   2 files changed, 55 insertions(+)
>>   create mode 100644 meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch
>>
>> diff --git a/meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch b/meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch
>> new file mode 100644
>> index 0000000..a638d58
>> --- /dev/null
>> +++ b/meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch
>> @@ -0,0 +1,54 @@
>> +Upstream-Status: Inappropriate [oe specific]
> >From the description, this sounds like an allocation error which can
> happen *anywhere* and is a problem that should be addressed upstream.
>
> This Upstream-Status field is therefore completely bogus. Its not
> inappropriate or oe specific. If you still believe it is, I'd like to
> hear more explanation.
>
> The abuses of this field are starting to really annoy me since this
> keeps happening.
>
> Cheers,
>
> Richard
>
>
>
>
>

Hi Richard,

The use of alloca() was introduced by an oe-specific patch from Khem Raj.

The patch is 
meta/recipes-core/systemd/systemd/systemd-pam-fix-fallocate.patch.
The upstream status of the above patch is as following.
        Upstream-Status: Denied [no desire for uclibc support]

That's why I use 'Inappropriate [oe specific]' in the Upstream-Status 
field of my patch.

And I just realized I forgot to also patch the journald-kmsg.c file. 
I'll send out a V2.

Best Regards,
Chen Qi

Re: [daisy][PATCH 1/1] systemd: do not use alloca() function in case of uclibc

[ChenQi]

On 06/03/2014 05:20 PM, ChenQi wrote:
> On 06/03/2014 05:06 PM, Richard Purdie wrote:
>> On Tue, 2014-06-03 at 15:42 +0800, Chen Qi wrote:
>>> The alloca() function allocates space in the stack frame of the caller,
>>> so using alloca(new_size - old_size) would possibly crash the stack,
>>> causing a segment fault error.
>>>
>>> This patch fixes the above problem by avoiding using this function in
>>> journal-file.c.
>>>
>>> [YOCTO #6201]
>>>
>>> Signed-off-by: Chen Qi <Qi.Chen@windriver.com>
>>> ---
>>>   .../0001-journal-file.c-do-not-use-alloca.patch    |   54 
>>> ++++++++++++++++++++
>>>   meta/recipes-core/systemd/systemd_211.bb           |    1 +
>>>   2 files changed, 55 insertions(+)
>>>   create mode 100644 
>>> meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch
>>>
>>> diff --git 
>>> a/meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch 
>>> b/meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch 
>>>
>>> new file mode 100644
>>> index 0000000..a638d58
>>> --- /dev/null
>>> +++ 
>>> b/meta/recipes-core/systemd/systemd/0001-journal-file.c-do-not-use-alloca.patch
>>> @@ -0,0 +1,54 @@
>>> +Upstream-Status: Inappropriate [oe specific]
>> >From the description, this sounds like an allocation error which can
>> happen *anywhere* and is a problem that should be addressed upstream.
>>
>> This Upstream-Status field is therefore completely bogus. Its not
>> inappropriate or oe specific. If you still believe it is, I'd like to
>> hear more explanation.
>>
>> The abuses of this field are starting to really annoy me since this
>> keeps happening.
>>
>> Cheers,
>>
>> Richard
>>
>>
>>
>>
>>
>
> Hi Richard,
>
> The use of alloca() was introduced by an oe-specific patch from Khem Raj.
>
> The patch is 
> meta/recipes-core/systemd/systemd/systemd-pam-fix-fallocate.patch.
> The upstream status of the above patch is as following.
>        Upstream-Status: Denied [no desire for uclibc support]
>
> That's why I use 'Inappropriate [oe specific]' in the Upstream-Status 
> field of my patch.
>
> And I just realized I forgot to also patch the journald-kmsg.c file. 
> I'll send out a V2.
>

Sorry for the confusion.

I just checked and  journald-kmsg.c doesn't have the same problem, as 
it's only allocating a small size of space.

      char *buf = alloca(sizeof(uint64_t));

So I think that's it.
I will also send out a patch for master branch.

Best Regards,
Chen Qi


> Best Regards,
> Chen Qi

Re: [daisy][PATCH 1/1] systemd: do not use alloca() function in case of uclibc

[Richard Purdie]

On Tue, 2014-06-03 at 17:20 +0800, ChenQi wrote:
> The use of alloca() was introduced by an oe-specific patch from Khem Raj.
> 
> The patch is 
> meta/recipes-core/systemd/systemd/systemd-pam-fix-fallocate.patch.
> The upstream status of the above patch is as following.
>         Upstream-Status: Denied [no desire for uclibc support]
> 
> That's why I use 'Inappropriate [oe specific]' in the Upstream-Status 
> field of my patch.
> 
> And I just realized I forgot to also patch the journald-kmsg.c file. 
> I'll send out a V2.

Please just update that patch then rather than patching code that we've
ready patched. Is this issue uclibc specific?

At the very least information like this needs to be put into the commit
message/patch header.

Cheers,

Richard

Re: [daisy][PATCH 1/1] systemd: do not use alloca() function in case of uclibc

[Holger Freyther]

Richard Purdie <richard.purdie@...> writes:

Hi,

> Please just update that patch then rather than patching code that we've
> ready patched. Is this issue uclibc specific?

the usage of alloca is due me and killing a lot of malloc's in
the hot path of log handling. I am late to these patches
but my two cents would be that if you use uclibc you really
want to have alloca for the speed.


holger