Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mc-butler opened this issue Mar 15, 2016 · 33 comments
Closed

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

mc-butler opened this issue Mar 15, 2016 · 33 comments
Assignees
Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress ver: 4.8.16 Reproducible in version 4.8.16
Milestone

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/3617
Reporter dmic (dmic@….ru)
Mentions dmic@….ru, Lena@….kiev.ua

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 [294908]:

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'

Note

Original attachments:

@mc-butler
Copy link
Author

Changed by Lena (Lena@….kiev.ua) on Mar 15, 2016 at 14:33 UTC (comment 1)

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);
     }

@mc-butler
Copy link
Author

Changed by Lena (Lena@….kiev.ua) on Mar 15, 2016 at 14:35 UTC

Eugene Grosbein's patch

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Mar 15, 2016 at 19:36 UTC (comment 2)

  • 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!

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Mar 15, 2016 at 22:57 UTC (comment 3)

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

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Mar 15, 2016 at 22:59 UTC

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Mar 15, 2016 at 23:05 UTC (comment 4)

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

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Mar 15, 2016 at 23:18 UTC (comment 3.5)

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

@mc-butler
Copy link
Author

Changed by zaytsev-work (@zyv) on Mar 16, 2016 at 9:04 UTC (comment 6)

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Mar 16, 2016 at 11:05 UTC (comment 6.7)

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.

@mc-butler
Copy link
Author

Changed by zaytsev-work (@zyv) on Mar 16, 2016 at 13:48 UTC (comment 7.8)

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.

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Mar 16, 2016 at 16:02 UTC (comment 8.9)

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

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Mar 16, 2016 at 16:04 UTC

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Mar 16, 2016 at 16:07 UTC (comment 10)

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

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Mar 16, 2016 at 20:43 UTC (comment 11)

Branch: 3617_freebsd_crash
Initial changeset: [13c897ddbf45fa56f27a7d6247f712eb901ed8ce]

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Mar 16, 2016 at 20:47 UTC (comment 12)

mooffie, thank you very much for your help!

@mc-butler
Copy link
Author

Changed by Lena (Lena@….kiev.ua) on Mar 17, 2016 at 2:58 UTC (comment 13)

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.

@mc-butler
Copy link
Author

Changed by Lena (Lena@….kiev.ua) on Mar 17, 2016 at 3:02 UTC (comment 14)

  • Cc changed from dmic@….ru to dmic@….ru, Lena@….kiev.ua

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Mar 17, 2016 at 5:49 UTC (comment 13.15)

Replying to Lena:

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

This is #3613.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Mar 17, 2016 at 6:43 UTC (comment 11.16)

Replying to zaytsev:

Branch: 3617_freebsd_crash

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

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Mar 17, 2016 at 8:00 UTC (comment 13.17)

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.

@mc-butler
Copy link
Author

Changed by zaytsev-work (@zyv) on Mar 17, 2016 at 8:29 UTC (comment 18)

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.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Mar 17, 2016 at 8:50 UTC (comment 18.19)

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.

@mc-butler
Copy link
Author

Changed by Lena (Lena@….kiev.ua) on Mar 17, 2016 at 9:40 UTC (comment 17.20)

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'

@mc-butler
Copy link
Author

Changed by Lena (Lena@….kiev.ua) on Mar 17, 2016 at 10:03 UTC (comment 21)

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

@mc-butler
Copy link
Author

Changed by zaytsev-work (@zyv) on Mar 17, 2016 at 13:01 UTC (comment 19.22)

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

@mc-butler
Copy link
Author

Changed by woodsb02 (woodsb02@….com) on Mar 17, 2016 at 23:03 UTC (comment 23)

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

@mc-butler
Copy link
Author

Changed by mooffie (@mooffie) on Mar 18, 2016 at 0:11 UTC (comment 18.24)

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.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Mar 18, 2016 at 9:53 UTC (comment 25)

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Mar 20, 2016 at 11:17 UTC (comment 26)

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Mar 20, 2016 at 11:18 UTC (comment 27)

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Mar 28, 2016 at 6:01 UTC (comment 28)

  • Branch state changed from on review to approved

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Mar 28, 2016 at 6:02 UTC (comment 29)

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

Merged to master: [990f6f1].

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Mar 28, 2016 at 6:03 UTC (comment 30)

  • Status changed from testing to closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress ver: 4.8.16 Reproducible in version 4.8.16
Development

No branches or pull requests

2 participants