Re: [Info-mtools] segfault in 4.0.29 mcopy

[Natanael Copa]

On Mon, 7 Jun 2021 14:26:39 +0200
Alain Knaff <alain@knaff.lu> wrote:

> Hi,
> 
> On 07/06/2021 13:48, Natanael Copa wrote:
> [...]
> > I don't think it is. I believe the command line from the script was:
> > 
> >   mcopy -i ${DESTDIR}/boot/grub/efi.img -s ${DESTDIR}/efi ::
> > 
> > From here: 
> > https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/scripts/mkimg.base.sh#L253
> >   
> 
> ok, good.
> 
> [...]
> > I was able to reproduce it with:
> > 
> >   mformat -i /tmp/efi.img -C -f 1440
> >   mcopy -i /tmp/efi.img /etc/issue ::
> >   Segmentation fault  
> 
> good
> 
> [...]
> >> Did you use any other compilation flags which might help me reproduce 
> >> this?  
> > 
> > From build log:
> > 
> > gcc -Os -fomit-frame-pointer -DHAVE_CONFIG_H -DSYSCONFDIR=\"/etc/mtools\" 
> > -DCPU_i586 -DVENDOR_alpine -DOS_linux_musl -Os -fomit-frame-pointer -g 
> > -Wall -fno-strict-aliasing -I.  -I.  -c strtonum.c                          
> >                                       
> > gcc -Os -fomit-frame-pointer -DHAVE_CONFIG_H -DSYSCONFDIR=\"/etc/mtools\" 
> > -DCPU_i586 -DVENDOR_alpine -DOS_linux_musl -Os -fomit-frame-pointer -g 
> > -Wall -fno-strict-aliasing -I.  -I.  -c mkmanifest.c -Os
> > 
> > So compiler flags are -Os -fomit-frame-pointer  
> 
> Good.
> 
> > 
> > Another thing I discovered is that -DOS_linux_musl does not set the 
> > OS_linux define, which I think it should.  
> 
> Does indeed not look so good :-(

This does not appear to affect OS_linux_gnu case due to sysincludes.h has this:

#ifdef OS_linux_gnu
/* RMS strikes again */
# ifndef OS_linux
#  define OS_linux
# endif
#endif

Which may indicate that those lines in configure.in may not work as intended:

[
host_os0=`echo $host_os | sed 's/-/_/g'`
host_os1=`echo $host_os0 | sed 's/\./_/g'`
host_os2=`echo $host_os0 | sed 's/^\([^.]*\)\..*$/\1/g'`
host_os3=`echo $host_os2 | sed 's/^\([^0-9]*\)[0-9]*$/\1/g'`
host_cpu1=`echo $host_cpu | sed 's/\./_/g'`
host_vendor1=`echo $host_vendor | sed 's/\./_/g'`
HOST_ID="-DCPU_$host_cpu1 -DVENDOR_$host_vendor1 -DOS_$host_os1"
if [ $host_os1 != $host_os2 ] ; then
        HOST_ID="$HOST_ID -DOS_$host_os2"
fi
if [ $host_os1 != $host_os3 ] && [ $host_os2 != $host_os3 ] ; then
        HOST_ID="$HOST_ID -DOS_$host_os3"
fi

Looking at the lines right after seems that host_os3 is supposed to be
"linux" in the linux case, but it isn't.

my_host_os=`echo $host_os1 $host_os2 $host_os3 | sort -u`
objs=`echo $srcdir/*.c | sed 's/\.c$/.o/' `
if [ "X$GCC" = "Xyes" ] ; then
    Wall=-Wall
    if [ "$host_os3" = sunos ] ; then
            Wall=""
    fi
    if [ "$host_os3" = ultrix ] ; then
            Wall=""
    fi
    if [ "$host_os3" = linux ] ; then
            CFLAGS="$CFLAGS -fno-strength-reduce"
    fi


So i think the host_os3 is not what the scripts expects.

DEBUG: host_os0=linux_musleabihf
DEBUG: host_os1=linux_musleabihf
DEBUG: host_os2=linux_musleabihf
DEBUG: host_os3=linux_musleabihf

thats other issue though...

> 
> [...]
> > I have experimented a bit with it and it seems like I am not able to
> > reproduce it when I build without -fomit-frame-pointer. Here is another 
> > backtrace without optimizations (but with -fomit-frame-pointer):  
> 
> That, or disabling XDF (see below)
> 
> [...]
> > Here is the output from valgrind:
> > ==37442== Memcheck, a memory error detector
> > ==37442== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
> > ==37442== Using Valgrind-3.17.0 and LibVEX; rerun with -h for copyright info
> > ==37442== Command: ./mcopy -i /tmp/efi.img /etc/issue ::
> > ==37442== 
> > ==37442== Conditional jump or move depends on uninitialised value(s)
> > ==37442==    at 0x11623C: try_device (init.c:181)  
> 
> Indeed, if XDF is disabled, Stream is uninitialized in line 181 of
> init.c (it would have been initialized by XdfOpen in line 172, if XDF
> was activated).
> 
> The fix here is to add initialization in line 159:
> 
>                 Stream_t *Stream=NULL;

This does fix the issue. Thank you!


-nc
> 
> And this missing initialization could indeed lead to random values in
> Stream.
> 
> Will be fixed in 4.0.30
> 
> Regards,
> 
> Alain