mc crash when copying files between panels (F5) under FreeBSD 9.3

[dmic]

I did the update on multiple computers under FreeBSD 9.3 and 10.1
Midnight Commander crashes on FreeBSD 9.3 and normaly runing on a FreeBSD 10.1 when copiing files between panels (F5)

FreeBSD 9.3-RELEASE-p35 FreeBSD 9.3-RELEASE-p35 #0 r294908:

GNU Midnight Commander 4.8.16
Built with GLib 2.46.2
Using the S-Lang library with terminfo database
With builtin Editor
With subshell support as default
With support for background operations
With mouse support on xterm
With internationalization support
With multiple codepages support
Virtual File Systems: cpiofs, tarfs, sfs, extfs, ftpfs, sftpfs, fish
Data types: char: 8; int: 32; long: 64; void *: 64; size_t: 64; off_t: 64;

# LC_MESSAGES=C mc -F
Root directory: /root

[System data]

Config directory: /usr/local/etc/mc/
Data directory: /usr/local/share/mc/
File extension handlers: /usr/local/libexec/mc/ext.d/
VFS plugins and scripts: /usr/local/libexec/mc/

extfs.d: /usr/local/libexec/mc/extfs.d/
fish: /usr/local/libexec/mc/fish/

[User data]

Config directory: /root/.config/mc/
Data directory: /root/.local/share/mc/

skins: /root/.local/share/mc/skins/
extfs.d: /root/.local/share/mc/extfs.d/
fish: /root/.local/share/mc/fish/
mcedit macros: /root/.local/share/mc/mc.macros
mcedit external macros: /root/.local/share/mc/mcedit/macros.d/macro.*

Cache directory: /root/.cache/mc/

# mc --configure-options

'--with-internal-edit' '--enable-charset' '--enable-nls' '--disable-vfs-smb' '--without-smb-configdir' '--without-smb-codepagedir' '--with-subshell' '--disable-x' '--with-screen=slang' '--with-slang-includes=/usr/local/include' '--prefix=/usr/local' '--localstatedir=/var' '--mandir=/usr/local/man' '--infodir=/usr/local/info/' '--build=amd64-portbld-freebsd9.3' 'build_alias=amd64-portbld-freebsd9.3' 'CC=cc' 'CFLAGS=-O2 -pipe -fstack-protector -fno-strict-aliasing' 'LDFLAGS= -L/usr/local/lib -fstack-protector' 'LIBS=' 'CPPFLAGS=-I/usr/local/include' 'CPP=cpp' 'PKG_CONFIG=pkgconf'

[Lena]

mc 4.8.15 worked OK. mc 4.8.16 coredumps:
Program terminated with signal 4, Illegal instruction.

#0  0x08074f04 in mc_open (vpath=0x289dd9c0, flags=2561) at interface.c:202
        ap = 0xbfbfc3d8 "╓\201"
        result = -1
        mode = 0
        path_element = (const vfs_path_element_t *) 0x8150040

#1  0x08101b7c in copy_file_file (tctx=0x28907fc0, ctx=0x289dbb80,
    src_path=0x289dd960 "/home/lena/a", dst_path=0x289dd9d0 "/home/lena/aa/a")
    at file.c:1702
#2  0x08104362 in panel_operate (source_panel=0x2893d800, operation=OP_COPY,
    force_single=0) at file.c:2893
#3  0x080fae27 in copy_cmd () at cmd.c:797
#4  0x08081daf in midnight_execute_cmd (sender=0x289ca700, command=21)
    at midnight.c:1140
#5  0x08082864 in midnight_callback (w=0x289890f0, sender=0x289ca700,
    msg=MSG_ACTION, parm=21, data=0x0) at midnight.c:1570
#6  0x0805ccae in send_message (w=0x289890f0, sender=0x289ca700,
    msg=MSG_ACTION, parm=21, data=0x0) at widget-common.h:168
#7  0x0805cc5b in buttonbar_call (bb=0x289ca700, i=4) at buttonbar.c:156
#8  0x0805cd42 in buttonbar_callback (w=0x289ca700, sender=0x0,
    msg=MSG_HOTKEY, parm=1005, data=0x0) at buttonbar.c:175
#9  0x0806113e in send_message (w=0x289ca700, sender=0x0, msg=MSG_HOTKEY,
    parm=1005, data=0x0) at widget-common.h:168
#10 0x0806198e in dlg_try_hotkey (h=0x289890f0, d_key=1005) at dialog.c:464
#11 0x08061aa9 in dlg_key_event (h=0x289890f0, d_key=1005) at dialog.c:509
#12 0x0806317a in dlg_process_event (h=0x289890f0, key=1005, event=0xbfbfeb10)
    at dialog.c:1236
#13 0x08061cd3 in frontend_dlg_run (h=0x289890f0) at dialog.c:570
#14 0x08063238 in dlg_run (h=0x289890f0) at dialog.c:1267
#15 0x0808187e in create_panels_and_run_mc () at midnight.c:955
#16 0x080831db in do_nc () at midnight.c:1815
#17 0x08053729 in main (argc=6, argv=0xbfbfec5c) at main.c:403

FreeBSD 8.4 i386, gcc.
Celeron D 3.20GHz Id = 0xf41 Family = f Model = 4 Stepping = 1
I removed "CPUTYPE?=prescott" from make.conf, recompiled mc - the same
"Program terminated with signal 4, Illegal instruction".

~ $ LANG=C mc --version
GNU Midnight Commander 4.8.16
Built with GLib 2.42.2
Using the S-Lang library with terminfo database
With builtin Editor
With subshell support as default
With support for background operations
With mouse support on xterm
With support for X11 events
With internationalization support
With multiple codepages support
Virtual File Systems: cpiofs, tarfs, sfs, extfs, ftpfs, sftpfs, fish
Data types: char: 8; int: 32; long: 32; void *: 32; size_t: 32; off_t: 64;

~ $ mc --configure-options
 '--with-internal-edit' '--enable-charset' '--enable-nls' '--disable-vfs-smb' '--without-smb-configdir' '--without-smb-codepagedir' '--with-subshell' '--enable-x' '--with-screen=slang' '--with-slang-includes=/usr/local/include' '--x-libraries=/usr/local/lib' '--x-includes=/usr/local/include' '--prefix=/usr/local' '--localstatedir=/var' '--mandir=/usr/local/man' '--infodir=/usr/local/info/' '--build=i386-portbld-freebsd8.4' 'build_alias=i386-portbld-freebsd8.4' 'CC=cc' 'CFLAGS=-pipe -g -fno-strict-aliasing' 'LDFLAGS= -L/usr/local/lib' 'LIBS=' 'CPPFLAGS=-I/usr/local/include' 'CPP=cpp' 'PKG_CONFIG=pkgconf'

Line 202 in /usr/ports/misc/mc/work/mc-4.8.16/lib/vfs/interface.c :

va_start (ap, flags);

This line 202 is in the function:

int
mc_open (const vfs_path_t * vpath, int flags, ...)
{
    int result = -1;
    mode_t mode = 0;
    const vfs_path_element_t *path_element;

    if (vpath == NULL)
        return -1;

    /* Get the mode flag */
    if (flags & O_CREAT)
    {
        va_list ap;
        va_start (ap, flags);
        mode = va_arg (ap, mode_t);
        va_end (ap);
    }

    path_element = vfs_path_get_by_index (vpath, -1);

Eugene Grosbein noticed that while compiling:

interface.c: In function 'mc_open':
interface.c:203: warning: 'mode_t' is promoted to 'int' when passed through '...'
interface.c:203: warning: (so you should pass 'int' not 'mode_t' to 'va_arg')
interface.c:203: note: if this code is reached, the program will abort

He offered a patch:

--- lib/vfs/interface.c.orig    2016-03-12 22:45:47.000000000 +0700
+++ lib/vfs/interface.c 2016-03-15 21:17:26.383826000 +0700
@@ -200,7 +200,7 @@ mc_open (const vfs_path_t * vpath, int f
     {
         va_list ap;
         va_start (ap, flags);
-        mode = va_arg (ap, mode_t);
+        mode = va_arg (ap, int);
         va_end (ap);
     }

[Lena]

Eugene Grosbein's patch

[zaytsev]

Holy shit, we should fix that for the next release. Thanks for the investigation and the patch!

[mooffie]

Here's a better patch. See explanation in its commit message.

@Lena: Could you please test the patch? It should work but I want to verify that GCC doesn't print a warning.

(As for why I used if else instead of the ternary operator in Gnulib's manual page: that's because of 'indent'.)

[mooffie]

(BTW, other ​places where we use va_arg all look kosher.)

[mooffie]

Replying to mooffie:

@Lena: Could you please test the patch? It should work but I want to verify that GCC doesn't print a warning.

(I see that somebody ​at a page I linked to in the patch's commit message says that "at this point we are getting GCC warnings", so let's indeed see what @Lena reports back.)

[zaytsev-work]

Hmmm, judging from the mailing list thread you linked to, I personally feel better about a configure-time solution...

[andrew_b]

Replying to zaytsev-work:

Hmmm, judging from the mailing list thread you linked to, I personally feel better about a configure-time solution...

sizeof() is compile-time operator. So optimizing compiler will leave only one branch in binary code.

[zaytsev-work]

Replying to andrew_b:

Replying to zaytsev-work:

Hmmm, judging from the mailing list thread you linked to, I personally feel better about a configure-time solution...

sizeof() is compile-time operator. So optimizing compiler will leave only one branch in binary code.

This is certainly true, but in the mailing list thread reference above, however, people are complaining that it will still produce a warning for the inactive branch (see comment:5), which is why they have chosen a configure-time solution in the end.

[mooffie]

Replying to zaytsev-work:

in the mailing list thread reference above, however, people are complaining that it will still produce a warning for the inactive branch (see comment:5), which is why they have chosen a configure-time solution in the end.

Right.

So I'm attaching an updated patch that uses their (Gnulib's) solution.

(For the record: alternatively, we can add "AC_CHECK_SIZEOF(int)" and "AC_CHECK_SIZEOF(mode_t)" to our configure.ac and do "#if SIZEOF_MODE_T < SIZEOF_INT" in our C.)

[mooffie]

(BTW, I didn't know if our "coding standards" call for an explicit cast when assigning 'int' to 'mode_t', so I left it out.)

[zaytsev]

Branch: 3617_freebsd_crash
Initial changeset: [13c897ddbf45fa56f27a7d6247f712eb901ed8ce]

[zaytsev]

mooffie, thank you very much for your help!

[Lena]

With the last patch (3617-mc_open-handle-varargs-mode_t-promotion-issue--v2.patch):

  CC       interface.lo
cc1: warning: -Wuninitialized is not supported without -O
interface.c: In function 'mc_open':
interface.c:206: error: expected specifier-qualifier-list before 'PROMOTED_MODE_T'
Makefile:524: recipe for target 'interface.lo' failed
gmake[3]: *** [interface.lo] Error 1

~ $ cc --version
cc (GCC) 4.2.1 20070831 patched [FreeBSD]

I just noticed that with the initial patch (interface.patch) mc does something funny in a "sh:" panel: takes much more time to connect, read directories and show the panel, and where filenames should be, it shows date-times and partial (truncated or something) filenames. I was afraid that it can damage data and downgraded to 4.8.15.

[andrew_b]

Replying to Lena:

it shows date-times and partial (truncated or something) filenames.

This is #3613.

[andrew_b]

Replying to zaytsev:

Branch: 3617_freebsd_crash

Sorry, I've deleted this branch accidentally.
@zaytsev: Yury, please push it again.

[mooffie]

Replying to Lena:

With the last patch (3617-mc_open-handle-varargs-mode_t-promotion-issue--v2.patch):

  CC       interface.lo
cc1: warning: -Wuninitialized is not supported without -O
interface.c: In function 'mc_open':
interface.c:206: error: expected specifier-qualifier-list before 'PROMOTED_MODE_T'

Did you re-generate the 'configure' script? You need to run 'autoreconf' (in the directory where 'configure.ac' resides).

If this sounds like Chinese to you, don't worry: you can instead use the tarball I uploaded here:

​http://www.typo.co.il/~mooffie/tmp/mc/mc-4.8.16-5-g55a208b.tar.gz

So please try it out (either the tarball or 'autoreconf') and let us know the result.

[zaytsev-work]

So please try it out (either the tarball or 'autoreconf') and let us know the result.

I'm afraid that simply doing autoreconf -vfi isn't going to work and you really need to run the autogen.sh for the moment. I'm not sure what the problem is, but it didn't work last time I've tried and I didn't have time to investigate what needs to be done to finally ditch autogen.sh and move to autoreconf.

Of course, I'll be only happy to know if the problem has resolved itself in the meantime.

[andrew_b]

Replying to zaytsev-work:

it didn't work last time I've tried and I didn't

This is very strange. I use autogen.sh every time when I build mc and it works fine for me.

[Lena]

Replying to mooffie:

Did you re-generate the 'configure' script?

I did "make clean" (nothing from the previous bulid remained) and used FreeBSD ports system to do everything from the very beginning. I just did it again, same error:

expected specifier-qualifier-list before 'PROMOTED_MODE_T'

[Lena]

Ah, I understood, the script is contained in the distribution tarball. I used your tarball, now everything works.

[zaytsev-work]

Replying to andrew_b:

Replying to zaytsev-work:

it didn't work last time I've tried and I didn't

This is very strange. I use autogen.sh every time when I build mc and it works fine for me.

This comment didn't pertain to autogen.sh, I talking about autoreconf not being enough and the necessity to use autogen.sh...

[woodsb02]

Note that FreeBSD bug report 208102 has been raised to fix this problem in the FreeBSD mc packages prior to the 4.8.17 release. Since FreeBSD ports use the release tarballs, the patch proposed for inclusion is based on "3617-mc_open-handle-varargs-mode_t-promotion-issue--v2.patch" but includes all the changes from re-running autogen.sh.

[mooffie]

Replying to zaytsev-work:

I'm afraid that simply doing autoreconf -vfi isn't going to work and you really need to run the autogen.sh for the moment.

Thanks for letting me know. (I always use 'autoreconf', but I did use './autogen.sh' initially; I did notice that 'autoreconf' doesn't update po/POTFILES.in and probably other things but it was good enough for me.)

Replying to woodsb02:

Note that FreeBSD bug report 208027 has been raised to fix this problem [...] the patch proposed for inclusion is based on "3617-mc_open-handle-varargs-mode_t-promotion-issue--v2.patch" [...]

So I take it as a silent confirmation that the patch is fine.

Andrew / Yury: Do you see any problem with the patch? Considering that it's from Gnulib and used in ​some ​projects I don't think there's a reason to worry.

[zaytsev]

I've only had time to re-push the branch so far, didn't get to testing it yet though, sorry.

[andrew_b]

Merged to master: [990f6f1a8bf04d0e32a9b6fbd015b743b37f8793].