Ticket #3617 (closed defect: fixed)

Opened 9 years ago

Last modified 9 years ago

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

Reported by: dmic Owned by: zaytsev
Priority: major Milestone: 4.8.17
Component: mc-core Version: 4.8.16
Keywords: Cc: dmic@…, Lena@…
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

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'

Attachments

interface.patch (288 bytes) - added by Lena 9 years ago.
Eugene Grosbein's patch
3617-mc_open-handle-varargs-mode_t-promotion-issue.patch (1.5 KB) - added by mooffie 9 years ago.
3617-mc_open-handle-varargs-mode_t-promotion-issue--v2.patch (3.9 KB) - added by mooffie 9 years ago.

Change History

comment:1 Changed 9 years ago by 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);
     }

Changed 9 years ago by Lena

Eugene Grosbein's patch

comment:2 Changed 9 years ago by zaytsev

  • Milestone changed from Future Releases to 4.8.17

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

comment:3 follow-up: ↓ 5 Changed 9 years ago by 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'.)

comment:4 Changed 9 years ago by mooffie

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

comment:5 in reply to: ↑ 3 Changed 9 years ago by 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


(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.)

Version 0, edited 9 years ago by mooffie (next)

comment:6 follow-up: ↓ 7 Changed 9 years ago by zaytsev-work

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

comment:7 in reply to: ↑ 6 ; follow-up: ↓ 8 Changed 9 years ago by 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.

comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 9 years ago by 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.

comment:9 in reply to: ↑ 8 Changed 9 years ago by 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.)

comment:10 Changed 9 years ago by 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.)

comment:11 follow-up: ↓ 16 Changed 9 years ago by zaytsev

Branch: 3617_freebsd_crash
Initial changeset: [13c897ddbf45fa56f27a7d6247f712eb901ed8ce]

comment:12 Changed 9 years ago by zaytsev

mooffie, thank you very much for your help!

comment:13 follow-ups: ↓ 15 ↓ 17 Changed 9 years ago by 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.

Last edited 9 years ago by Lena (previous) (diff)

comment:14 Changed 9 years ago by Lena

  • Cc Lena@… added

comment:15 in reply to: ↑ 13 Changed 9 years ago by andrew_b

Replying to Lena:

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

This is #3613.

comment:16 in reply to: ↑ 11 Changed 9 years ago by andrew_b

Replying to zaytsev:

Branch: 3617_freebsd_crash

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

comment:17 in reply to: ↑ 13 ; follow-up: ↓ 20 Changed 9 years ago by 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.

comment:18 follow-ups: ↓ 19 ↓ 24 Changed 9 years ago by 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.

comment:19 in reply to: ↑ 18 ; follow-up: ↓ 22 Changed 9 years ago by 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.

comment:20 in reply to: ↑ 17 Changed 9 years ago by 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'

comment:21 Changed 9 years ago by Lena

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

comment:22 in reply to: ↑ 19 Changed 9 years ago by 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...

comment:23 Changed 9 years ago by woodsb02

Note that FreeBSD bug report 208027 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.
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=208027

Last edited 9 years ago by woodsb02 (previous) (diff)

comment:24 in reply to: ↑ 18 Changed 9 years ago by 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.

comment:25 Changed 9 years ago by zaytsev

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

comment:26 Changed 9 years ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from no branch to on review

comment:27 Changed 9 years ago by andrew_b

  • Owner set to zaytsev
  • Status changed from new to assigned

comment:28 Changed 9 years ago by andrew_b

  • Branch state changed from on review to approved

comment:29 Changed 9 years ago by andrew_b

  • Status changed from assigned to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

comment:30 Changed 9 years ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.