Bug 6201

Summary: systemd-pam-fix-fallocate.patch uses wrong #define and can easily blow stack
Product: [Build System, Metadata & Runtime] OE-Core Reporter: Matt Cowell <matt.cowell>
Component: coreAssignee: Chen Qi <Qi.Chen>
Status: VERIFIED FIXED QA Contact: Costin <costin.c.constantin>
Severity: normal    
Priority: Medium+ CC: alexandru.c.georgescu, meta.mr.watcher, meta.watcher, sgw
Version: 1.6   
Target Milestone: 1.6.1   
Hardware: All   
OS: Multiple   
Whiteboard:
OS type for building Yocto: --- Type of Regression: ---
Verified: Documentation change: No (bug/feature does not impact docs)

Description Matt Cowell 2014-04-21 19:05:53 UTC
systemd-pam-fix-fallocate.patch adds an #ifdef for HAVE_POSIX_ALLOCATE, which should be HAVE_POSIX_FALLOCATE instead.  Additionally, the code this patch adds for "alloca(new_size - old_size)" can easily allocate over 8MB of space (it does on my system), which will crash with the default stack ulimit of 8MB.

Fixing the #ifdef resolves the issue for eglibc systems.

Details on crash:

Program received signal SIGSEGV, Segmentation fault.
journal_file_allocate (f=f@entry=0x47d70, offset=<optimized out>,
    size=size@entry=5344)
    at /usr/src/debug/systemd/1_211+gitAUTOINC+3a450ec5c6-r1/git/src/journal/journal-file.c:378
378                    off_t oldpos = lseek(f->fd, 0, SEEK_CUR);
(gdb) l
373     #else
374            /* Use good old method to write zeros into the journal file
375               perhaps very inefficient yet working. */
376            if(new_size > old_size) {
377                    char *buf = alloca(new_size - old_size);
378                    off_t oldpos = lseek(f->fd, 0, SEEK_CUR);
379                    bzero(buf, new_size - old_size);
380                    lseek(f->fd, old_size, SEEK_SET);
381                    r = write(f->fd, buf, new_size - old_size);
382                    lseek(f->fd, oldpos, SEEK_SET);
(gdb) p new_size
$1 = 8388608
(gdb) p old_size
$2 = 240
Comment 1 Chen Qi 2014-05-14 02:11:24 UTC
(In reply to comment #0)
> systemd-pam-fix-fallocate.patch adds an #ifdef for HAVE_POSIX_ALLOCATE,
> which should be HAVE_POSIX_FALLOCATE instead.  Additionally, the code this
> patch adds for "alloca(new_size - old_size)" can easily allocate over 8MB of
> space (it does on my system), which will crash with the default stack ulimit
> of 8MB.
> 
> Fixing the #ifdef resolves the issue for eglibc systems.
> 
> Details on crash:
> 
> Program received signal SIGSEGV, Segmentation fault.
> journal_file_allocate (f=f@entry=0x47d70, offset=<optimized out>,
>     size=size@entry=5344)
>     at
> /usr/src/debug/systemd/1_211+gitAUTOINC+3a450ec5c6-r1/git/src/journal/
> journal-file.c:378
> 378                    off_t oldpos = lseek(f->fd, 0, SEEK_CUR);
> (gdb) l
> 373     #else
> 374            /* Use good old method to write zeros into the journal file
> 375               perhaps very inefficient yet working. */
> 376            if(new_size > old_size) {
> 377                    char *buf = alloca(new_size - old_size);
> 378                    off_t oldpos = lseek(f->fd, 0, SEEK_CUR);
> 379                    bzero(buf, new_size - old_size);
> 380                    lseek(f->fd, old_size, SEEK_SET);
> 381                    r = write(f->fd, buf, new_size - old_size);
> 382                    lseek(f->fd, oldpos, SEEK_SET);
> (gdb) p new_size
> $1 = 8388608
> (gdb) p old_size
> $2 = 240


Hi Matt,

It seems that you've already got a solution, right? Are you going to send out a patch?

Is this problem specific to 1.6 branch? Does our master branch also have this problem?

It would be really appreciated if you could give some more information.

Best Regards,
Chen Qi
Comment 2 Matt Cowell 2014-05-14 17:53:12 UTC
Unfortunately I do not run uClibc, so I did not actually fix the code that is using alloca.  I simply fixed the #ifdef, which resolves the issue for eglibc systems where HAVE_POSIX_FALLOCATE will be defined.

It appears that this #ifdef was fixed (same as I did) in master on 4/29 by this commit:
http://git.yoctoproject.org/cgit.cgi/poky/diff/?id=936218e789277c6939535779c240d386e18c8891

Looking at the git logs, it appears that this patch was introduced in 1.4 and also affects 1.5 and 1.6.  However, I believe that something in systemd changed between versions 199 and 211 which now reserves 8MB for the journal file at boot, since we did not see the crash in 1.4.  Therefore, there may still be an outstanding issue for uClibc systems in the 1.6 release (and possibly 1.5), but I cannot confirm.
Comment 3 Chen Qi 2014-05-23 06:50:24 UTC
(In reply to comment #2)
> Unfortunately I do not run uClibc, so I did not actually fix the code that
> is using alloca.  I simply fixed the #ifdef, which resolves the issue for
> eglibc systems where HAVE_POSIX_FALLOCATE will be defined.
> 
> It appears that this #ifdef was fixed (same as I did) in master on 4/29 by
> this commit:
> http://git.yoctoproject.org/cgit.cgi/poky/diff/
> ?id=936218e789277c6939535779c240d386e18c8891
> 
> Looking at the git logs, it appears that this patch was introduced in 1.4
> and also affects 1.5 and 1.6.  However, I believe that something in systemd
> changed between versions 199 and 211 which now reserves 8MB for the journal
> file at boot, since we did not see the crash in 1.4.  Therefore, there may
> still be an outstanding issue for uClibc systems in the 1.6 release (and
> possibly 1.5), but I cannot confirm.

Hi Matt,

Thanks for your info.
I'm going to look into this problem to make sure everything is correctly fixed.

I'm now firing up two builds. Maybe this question is stupid, but is there any easy way to reproduce the problem?

Best Regards,
Chen Qi
Comment 4 Chen Qi 2014-05-26 05:24:25 UTC
Hi Matt,

I still cannot find a reasonable way to reproduce it.
Could you please tell me how you do it?

Thanks,
Chen Qi
Comment 5 Chen Qi 2014-06-11 07:23:59 UTC
Module: openembedded-core.git
Branch: daisy
Commit: 96b6a2d446d28eabd9a943f5f2b5af12c24a7dbb
URL:    http://git.openembedded.org/?p=openembedded-core.git&a=commit;h=96b6a2d446d28eabd9a943f5f2b5af12c24a7dbb

Author: Chen Qi <Qi.Chen@windriver.com>
Date:   Wed Jun  4 17:47:08 2014 +0800

systemd: update a uclibc specific patch to avoid segment fault

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>
Signed-off-by: Saul Wold <sgw@linux.intel.com>



Module: openembedded-core.git
Branch: master
Commit: c69816d2bf84369ba578bf9d92e01c9d91351a64
URL:    http://git.openembedded.org/?p=openembedded-core.git&a=commit;h=c69816d2bf84369ba578bf9d92e01c9d91351a64

Author: Chen Qi <Qi.Chen@windriver.com>
Date:   Wed Jun  4 17:47:25 2014 +0800

systemd: update a uclibc specific patch to avoid segment fault error

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>
Signed-off-by: Saul Wold <sgw@linux.intel.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
Comment 6 Costin 2014-08-18 09:23:02 UTC
Hello Chen,

I pulled to last version of master b7e451894c2c9b02570deb053f76bae611968151
and checked for the patch you said about:
meta/recipes-core/systemd/systemd/systemd-pam-fix-fallocate.patch
At this point it is as you said in the last comment. Reading through out the bug I understood the mods. you did so I will mark this bug as verified. 

Regards,

Costin