Runtime error when invoking complete in save-entry

jdujava commented 9 months ago

Expected Behaviour

Invoking complete-query in save-entry (called from Attachments view) shouldn't emit runtime error message.

Actual Behaviour

When pressing <Tab> (trying to invoke complete via binding bind editor <Tab> complete-query) on an empty save-entry prompt results in following message (but neomutt doesn't crash)

Save to file: browser/complete.c:139:5: runtime error: null pointer passed as argument 1, which is declared to never be null

Steps to Reproduce

using v enter Attachments view on any message
using s open save-entry prompt
by default there is ./ inserted, so delete it to obtain an empty prompt
invoking complete-query (for example with <Tab> and binding mentioned earlier) results in the error message

How often does this happen?

My limited testing suggests that it happens always, but only on the first try after opening neomutt.

When did it start to happen?

I noticed it already on the previous release NeoMutt 2023-12-21.

NeoMutt Version

$ neomutt -v
NeoMutt 20240201
Copyright (C) 2015-2024 Richard Russon and friends
NeoMutt comes with ABSOLUTELY NO WARRANTY; for details type 'neomutt -vv'.
NeoMutt is free software, and you are welcome to redistribute it
under certain conditions; type 'neomutt -vv' for details.

System: Linux 6.7.2-arch1-2 (x86_64)
ncurses: ncurses 6.4.20230520 (compiled with 6.4.20230520)
libidn2: 2.3.4 (compiled with 2.3.4)
GPGME: 1.23.2
GnuTLS: 3.8.3
libnotmuch: 5.6.0
storage: kyotocabinet, gdbm, lmdb
compression: lz4, zlib, zstd

Configure options: --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib --gpgme --sqlite --autocrypt --lua --notmuch --gss --gnutls --sasl --with-ui=ncurses --with-idn2=/usr --disable-idn --idn2 --lmdb --kyotocabinet --gdbm --ubsan --lz4 --zlib --zstd

Compilation CFLAGS: -march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions         -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security         -fstack-clash-protection -fcf-protection -g -ffile-prefix-map=/build/neomutt/src=/usr/src/debug/neomutt -flto=auto -fno-delete-null-pointer-checks -D_ALL_SOURCE=1 -D_GNU_SOURCE=1 -D__EXTENSIONS__ -D_XOPEN_SOURCE_EXTENDED -I/usr/include/lua5.3 -I/usr/include -I/usr/include -I/usr/include -DNCURSES_WIDECHAR -I/usr/include -I/usr/include/p11-kit-1 -I/usr/include -I/usr/include -fsanitize=undefined -fno-omit-frame-pointer -fno-common -g -O0

Compile options:
  +autocrypt +fcntl -flock -fmemopen +futimens +getaddrinfo +gnutls +gpgme
  -gsasl +gss +hcache -homespool +idn +inotify -locales_hack +lua -mixmaster
  +nls +notmuch -openssl +pgp +regex +sasl +smime +sqlite +truecolor

Devel options:
  ubsan

MAILPATH="/var/mail"
PKGDATADIR="/usr/share/neomutt"
SENDMAIL="/usr/sbin/sendmail"
SYSCONFDIR="/etc"

To learn more about NeoMutt, visit: https://neomutt.org
If you find a bug in NeoMutt, please raise an issue at:
    https://github.com/neomutt/neomutt/issues
or send an email to: <neomutt-devel@neomutt.org>

MeindertKempe commented 9 months ago

That appears to be a message from ubsan, at the indicated line browser/complete.c:139:5 there is a call to memcpy, the first argument is allocated the line before using mutt_mem_realloc. With the buffer being empty the size passed to mutt_mem_realloc is 0, which results in memcpy using a NULL pointer as the destination buffer, since the size passed to memcpy is also 0 I don't believe particularly disastrous is likely to happen, but it is undefined behaviour to pass a NULL pointer to memcpy.

I'm not entirely certain how this complete function is supposed to function, but I believe it would be relatively simple to add a simple check for the case of 0 length.

flatcap commented 9 months ago

Thanks for the bug report; I haven't had a chance to investigate yet.

As you've already noted, a simple check would stop the problem. I'll certainly add something like that to the code before the next release.

However...

What I'd love to know is why we're getting a NULL there.

If someone has a bit of time to dig into the code, it'd really be appreciated. Feel free to ask lots of questions here, or on IRC: #neomutt on irc.libera.chat (web client)

Thanks!

roccoblues commented 8 months ago

I can't seem reproduce this.

With ubsan enabled I actually get a different error on every completion, not just empty. Maybe because I'm on macOS.

 editor/functions.c:187:12: runtime error: call to function complete_file_simple through pointer to incorrect function type 'enum FunctionRetval (*)(struct EnterWindowData *, int)'
             complete.c:114: note: complete_file_simple defined here
                                                                    SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior editor/functions.c:187:12

Which I can fix with:

diff --git a/browser/complete.c b/browser/complete.c
index f6dca66e1..673668a44 100644
--- a/browser/complete.c
+++ b/browser/complete.c
@@ -110,7 +110,7 @@ int complete_file_mbox(struct EnterWindowData *wdata, int op)
 /**
  * complete_file_simple - Complete a filename - Implements ::complete_function_t - @ingroup complete_api
  */
-int complete_file_simple(struct EnterWindowData *wdata, int op)
+enum FunctionRetval complete_file_simple(struct EnterWindowData *wdata, int op)
 {
   if (!wdata || ((op != OP_EDITOR_COMPLETE) && (op != OP_EDITOR_COMPLETE_QUERY)))
     return FR_NO_ACTION;

jdujava commented 4 months ago

I can no longer reproduce this in NeoMutt 20240425.

Should I close this now?

roccoblues commented 4 months ago

I can no longer reproduce this in NeoMutt 20240425.

🎉

Should I close this now?

Thanks for getting back to us! I'll close this.

neomutt / neomutt