"illegal hardware instruction" on macOS compiled nnn when invoking any plugin #786
Comments
|
Don't have the time to read the issue atm, but this sounds awfully a lot like #766 OP of that post never gave us a full backtrace, could you maybe provide one so we can have a look at what's going on? |
|
Here's Here's frame 3: And Happy to help as much as possible but as I'm sure you can tell I'm way out of my depth here! |
|
Okay interesting development! I noticed lldb said "compiled with optimization - stepping may behave oddly ..." but I just did |
|
I think the tmp path length is the problem. Can you please apply the following patch, compile in debug mode and share the logs: diff --git a/src/nnn.c b/src/nnn.c
index 7b108b6..247e572 100644
--- a/src/nnn.c
+++ b/src/nnn.c
@@ -7492,6 +7492,8 @@ static bool set_tmp_path(void)
}
tmpfplen = (uchar)xstrsncpy(g_tmpfpath, path, TMP_LEN_MAX);
+ DPRINTF_S(g_tmpfpath);
+ DPRINTF_U(tmpfplen);
return TRUE;
}
|
|
I think you're right. Here's the output: |
|
But the following are the same on my system too! and I don't see this. Please try this: diff --git a/src/nnn.c b/src/nnn.c
index 7b108b6..8d25138 100644
--- a/src/nnn.c
+++ b/src/nnn.c
@@ -907,7 +907,7 @@ static void *xrealloc(void *pcur, size_t len)
* Always null ('\0') terminates if both src and dest are valid pointers.
* Returns the number of bytes copied including terminating null byte.
*/
-static size_t xstrsncpy(char *restrict dst, const char *restrict src, size_t n)
+static size_t xstrsncpy(char *dst, const char *src, size_t n)
{
char *end = memccpy(dst, src, '\0', n);
|
|
The issue still occurs with the above change and the debug log output is the same. One thing I just noticed is that could this be relevant? |
|
I'll try to repro that using a symlink. For now, I'll have to retire. Thanks for all the details. We'll get back if we need anything else. |
|
Great, thank you! Compiling with a lower optimization level as a workaround means I'm able to use plugins and enjoy the awesome icons. 😎 |
|
You can try removing both the const and restrict qualifiers and give it a shot. It's really embarrassing not to have the HW and request for details repeatedly. :( |
|
I went so sleep after my message. I want to say thank you for providing the backtrace <3 it is very helpful. I'm not familiar with this section of code in particular. I peeked at the relevant code and it's using a various "shared" variables that I'm also not familiar with... Therefore I feel it would save a lot of time to test it on the hardware rather than just looking at the code... Either you can hit us up on https://gitter.im/jarun/nnn and we can remotely debug it with |
|
I tried the symlink variant now and didn't see the problem. As your tests indicate the compiler is probably handling @zmwangx can you please take a look? |
|
Can repro with Aside: You can find the actual compiler invocation at End of aside. Not sure why even |
|
(I only read the top of the thread so I might have been repeating some known things and ignoring some later developments. If there's something I should know, please kindly point me to it.) |
This is something I want to pursue. Seems something specific to the code generated on macOS. Interesting. I can surely wait till the weekend for this. :) |
|
Sure. I'll also read up on what you've found out thus far later. Btw, if you have the time and are feeling adventurous, I heard there's a trick you can use to SSH into GitHub Actions' macOS instances for some live debugging. Something like this: https://dev.to/retyui/how-debugging-github-actions-with-ssh-273n |
|
I tried that already as part of #766 investigation. Didn't see the issue. |
|
@prurph in the meantime, please check if removing both the |
|
Interesting, I wonder what’s different about MacStadium’s Mac Minis. Or maybe it’s something about the hypervisor. |
|
Also adding @saagarjha for some possible insight into the problem. |
|
@jarun removing |
|
OK. So you mean the program didn't even compile for you to test it, right? |
|
No, I'm sorry I should have been clearer. It compiled with warnings, but the problem still occurred when plugins were invoked. All of the warnings were |
|
OK. Are you able to see the help screen ( |
|
Interestingly, both the help and file stat screens work correctly. |
|
OK. Can you try |
|
Same result with |
|
Update: ignore this. I understand what you meant. |
|
What is the output of the following? |
|
Please try the following patch: diff --git a/src/nnn.c b/src/nnn.c
index 7b108b6..5a6e91b 100644
--- a/src/nnn.c
+++ b/src/nnn.c
@@ -4710,10 +4710,10 @@ static bool run_cmd_as_plugin(const char *file, char *runfile, uchar flags)
static bool plctrl_init(void)
{
- snprintf(g_buf, CMD_LEN_MAX, "nnn-pipe.%d", getpid());
/* g_tmpfpath is used to generate tmp file names */
g_tmpfpath[tmpfplen - 1] = '\0';
- mkpath(g_tmpfpath, g_buf, g_pipepath);
+ size_t r = mkpath(g_tmpfpath, "nnn-pipe.", g_pipepath);
+ xstrsncpy(g_pipepath + r - 1, xitoa(getpid()), TMP_LEN_MAX - r);
setenv(env_cfg[NNN_PIPE], g_pipepath, TRUE);
return EXIT_SUCCESS;This follows the same logic that |
|
If this doesn't work, I believe I do have a 100% way of making this work. But let's give the above patch a try first. |
|
Unfortunately, the above also doesn't work.
|
|
OK. I think now we need to try what i hinted at earlier. |
|
Please try the following patch: diff --git a/src/nnn.c b/src/nnn.c
index 8328718..8ada5ee 100644
--- a/src/nnn.c
+++ b/src/nnn.c
@@ -312,7 +312,6 @@ typedef struct {
/* Non-persistent program-internal states */
typedef struct {
- uint pluginit : 1; /* Plugin framework initialized */
uint interrupt : 1; /* Program received an interrupt */
uint rangesel : 1; /* Range selection on */
uint move : 1; /* Move operation */
@@ -332,7 +331,7 @@ typedef struct {
uint stayonsel : 1; /* Disable auto-proceed on select */
uint dirctx : 1; /* Show dirs in context color */
uint uidgid : 1; /* Show owner and group info */
- uint reserved : 10; /* Adjust when adding/removing a field */
+ uint reserved : 11; /* Adjust when adding/removing a field */
} runstate;
/* Contexts or workspaces */
@@ -4708,15 +4707,13 @@ static bool run_cmd_as_plugin(const char *file, char *runfile, uchar flags)
return TRUE;
}
-static bool plctrl_init(void)
+static void plctrl_init(void)
{
- snprintf(g_buf, CMD_LEN_MAX, "nnn-pipe.%d", getpid());
/* g_tmpfpath is used to generate tmp file names */
g_tmpfpath[tmpfplen - 1] = '\0';
- mkpath(g_tmpfpath, g_buf, g_pipepath);
+ size_t r = mkpath(g_tmpfpath, "nnn-pipe.", g_pipepath);
+ xstrsncpy(g_pipepath + r - 1, xitoa(getpid()), TMP_LEN_MAX - r);
setenv(env_cfg[NNN_PIPE], g_pipepath, TRUE);
-
- return EXIT_SUCCESS;
}
static void rmlistpath()
@@ -4803,11 +4800,6 @@ static bool run_selected_plugin(char **path, const char *file, char *runfile, ch
bool cmd_as_plugin = FALSE;
uchar flags = 0;
- if (!g_state.pluginit) {
- plctrl_init();
- g_state.pluginit = 1;
- }
-
if (*file == '_') {
flags = F_MULTI | F_CONFIRM;
@@ -7511,8 +7503,7 @@ static void cleanup(void)
if (g_state.autofifo)
unlink(fifopath);
#endif
- if (g_state.pluginit)
- unlink(g_pipepath);
+ unlink(g_pipepath);
#ifdef DBGMODE
disabledbg();
#endif
@@ -7813,11 +7804,11 @@ int main(int argc, char *argv[])
DPRINTF_S(getenv("PWD"));
+ plctrl_init();
+
#ifndef NOFIFO
/* Create fifo */
if (g_state.autofifo) {
- g_tmpfpath[tmpfplen - 1] = '\0';
-
size_t r = mkpath(g_tmpfpath, "nnn-fifo.", g_buf);
xstrsncpy(g_buf + r - 1, xitoa(getpid()), PATH_MAX - r); |
|
That exact patch yields a compilation error: I tried this: @@ -7321,11 +7313,7 @@ malloc_2:
free(paths[i]);
malloc_1:
if (msgnum) {
- if (g_state.pluginit) {
- printmsg(messages[msgnum]);
- xdelay(XDELAY_INTERVAL_MS);
- } else
- fprintf(stderr, "%s\n", messages[msgnum]);
+ fprintf(stderr, "%s\n", messages[msgnum]);
}
free(input);
free(paths);and it compiled, but |
|
My previous commit removes the flag so if you update to master the it would work. The crash is strange! You mentioned |
|
Wait, there's some issue...
|
|
Sorry, I guess I am sleepy. Ignore the above comment. Let's try the next version. |
|
Please upgrade to master and try the following patch: diff --git a/src/nnn.c b/src/nnn.c
index 8328718..f129ca3 100644
--- a/src/nnn.c
+++ b/src/nnn.c
@@ -312,7 +312,6 @@ typedef struct {
/* Non-persistent program-internal states */
typedef struct {
- uint pluginit : 1; /* Plugin framework initialized */
uint interrupt : 1; /* Program received an interrupt */
uint rangesel : 1; /* Range selection on */
uint move : 1; /* Move operation */
@@ -332,7 +331,7 @@ typedef struct {
uint stayonsel : 1; /* Disable auto-proceed on select */
uint dirctx : 1; /* Show dirs in context color */
uint uidgid : 1; /* Show owner and group info */
- uint reserved : 10; /* Adjust when adding/removing a field */
+ uint reserved : 11; /* Adjust when adding/removing a field */
} runstate;
/* Contexts or workspaces */
@@ -441,13 +440,13 @@ static struct sigaction oldsighup;
static struct sigaction oldsigtstp;
/* For use in functions which are isolated and don't return the buffer */
-static char g_buf[CMD_LEN_MAX] __attribute__ ((aligned));
+static char g_buf[CMD_LEN_MAX];
/* Buffer to store tmp file path to show selection, file stats and help */
-static char g_tmpfpath[TMP_LEN_MAX] __attribute__ ((aligned));
+static char g_tmpfpath[TMP_LEN_MAX];
/* Buffer to store plugins control pipe location */
-static char g_pipepath[TMP_LEN_MAX] __attribute__ ((aligned));
+static char g_pipepath[TMP_LEN_MAX];
/* Non-persistent runtime states */
static runstate g_state;
@@ -4708,17 +4707,6 @@ static bool run_cmd_as_plugin(const char *file, char *runfile, uchar flags)
return TRUE;
}
-static bool plctrl_init(void)
-{
- snprintf(g_buf, CMD_LEN_MAX, "nnn-pipe.%d", getpid());
- /* g_tmpfpath is used to generate tmp file names */
- g_tmpfpath[tmpfplen - 1] = '\0';
- mkpath(g_tmpfpath, g_buf, g_pipepath);
- setenv(env_cfg[NNN_PIPE], g_pipepath, TRUE);
-
- return EXIT_SUCCESS;
-}
-
static void rmlistpath()
{
if (listpath) {
@@ -4803,11 +4791,6 @@ static bool run_selected_plugin(char **path, const char *file, char *runfile, ch
bool cmd_as_plugin = FALSE;
uchar flags = 0;
- if (!g_state.pluginit) {
- plctrl_init();
- g_state.pluginit = 1;
- }
-
if (*file == '_') {
flags = F_MULTI | F_CONFIRM;
@@ -7511,8 +7494,7 @@ static void cleanup(void)
if (g_state.autofifo)
unlink(fifopath);
#endif
- if (g_state.pluginit)
- unlink(g_pipepath);
+ unlink(g_pipepath);
#ifdef DBGMODE
disabledbg();
#endif
@@ -7536,6 +7518,7 @@ int main(int argc, char *argv[])
#ifndef NORL
bool rlhist = FALSE;
#endif
+ size_t r;
while ((opt = (env_opts_id > 0
? env_opts[--env_opts_id]
@@ -7813,14 +7796,19 @@ int main(int argc, char *argv[])
DPRINTF_S(getenv("PWD"));
+ /* g_tmpfpath is used to generate tmp file names */
+ fd = getpid();
+ g_tmpfpath[tmpfplen - 1] = '\0';
+ r = mkpath(g_tmpfpath, "nnn-pipe.", g_pipepath);
+ xstrsncpy(g_pipepath + r - 1, xitoa(fd), TMP_LEN_MAX - r);
+ setenv(env_cfg[NNN_PIPE], g_pipepath, TRUE);
+
#ifndef NOFIFO
/* Create fifo */
if (g_state.autofifo) {
- g_tmpfpath[tmpfplen - 1] = '\0';
-
- size_t r = mkpath(g_tmpfpath, "nnn-fifo.", g_buf);
+ r = mkpath(g_tmpfpath, "nnn-fifo.", g_buf);
- xstrsncpy(g_buf + r - 1, xitoa(getpid()), PATH_MAX - r);
+ xstrsncpy(g_buf + r - 1, xitoa(fd), PATH_MAX - r);
setenv("NNN_FIFO", g_buf, TRUE);
}
|
|
Crashes on startup: |
|
This looks like a buffer overflow. Try turning in Address Sanitizer to see where it is. |
|
When I turn on address sanitizer nnn starts properly AND the original issue disappears: I'm able to invoke plugins and they work instead of crashing nnn. Just to be sure I'm doing this right, this is the invocation I used: (exactly what so it seems like whatever changes the address sanitizer makes also "fix" things. |
|
Not sure if this is because of the length of the globals. I can try to allocate the 64 bye ones dynamically and see how it goes. |
|
Please try this version (last one): diff --git a/src/nnn.c b/src/nnn.c
index 8328718..7311620 100644
--- a/src/nnn.c
+++ b/src/nnn.c
@@ -312,7 +312,6 @@ typedef struct {
/* Non-persistent program-internal states */
typedef struct {
- uint pluginit : 1; /* Plugin framework initialized */
uint interrupt : 1; /* Program received an interrupt */
uint rangesel : 1; /* Range selection on */
uint move : 1; /* Move operation */
@@ -332,7 +331,7 @@ typedef struct {
uint stayonsel : 1; /* Disable auto-proceed on select */
uint dirctx : 1; /* Show dirs in context color */
uint uidgid : 1; /* Show owner and group info */
- uint reserved : 10; /* Adjust when adding/removing a field */
+ uint reserved : 11; /* Adjust when adding/removing a field */
} runstate;
/* Contexts or workspaces */
@@ -415,6 +414,9 @@ static char *listroot;
static char *plgpath;
static char *pnamebuf, *pselbuf;
static char *mark;
+static char *g_tmpfpath;
+static char *g_pipepath;
+
#ifndef NOFIFO
static char *fifopath;
#endif
@@ -441,13 +443,7 @@ static struct sigaction oldsighup;
static struct sigaction oldsigtstp;
/* For use in functions which are isolated and don't return the buffer */
-static char g_buf[CMD_LEN_MAX] __attribute__ ((aligned));
-
-/* Buffer to store tmp file path to show selection, file stats and help */
-static char g_tmpfpath[TMP_LEN_MAX] __attribute__ ((aligned));
-
-/* Buffer to store plugins control pipe location */
-static char g_pipepath[TMP_LEN_MAX] __attribute__ ((aligned));
+static char g_buf[CMD_LEN_MAX];
/* Non-persistent runtime states */
static runstate g_state;
@@ -4708,17 +4704,6 @@ static bool run_cmd_as_plugin(const char *file, char *runfile, uchar flags)
return TRUE;
}
-static bool plctrl_init(void)
-{
- snprintf(g_buf, CMD_LEN_MAX, "nnn-pipe.%d", getpid());
- /* g_tmpfpath is used to generate tmp file names */
- g_tmpfpath[tmpfplen - 1] = '\0';
- mkpath(g_tmpfpath, g_buf, g_pipepath);
- setenv(env_cfg[NNN_PIPE], g_pipepath, TRUE);
-
- return EXIT_SUCCESS;
-}
-
static void rmlistpath()
{
if (listpath) {
@@ -4803,11 +4788,6 @@ static bool run_selected_plugin(char **path, const char *file, char *runfile, ch
bool cmd_as_plugin = FALSE;
uchar flags = 0;
- if (!g_state.pluginit) {
- plctrl_init();
- g_state.pluginit = 1;
- }
-
if (*file == '_') {
flags = F_MULTI | F_CONFIRM;
@@ -7491,6 +7471,9 @@ static bool set_tmp_path(void)
return FALSE;
}
+ g_tmpfpath = (char *)calloc(1, TMP_LEN_MAX);
+ if (!g_tmpfpath)
+ return FALSE;
tmpfplen = (uchar)xstrsncpy(g_tmpfpath, path, TMP_LEN_MAX);
return TRUE;
}
@@ -7511,8 +7494,9 @@ static void cleanup(void)
if (g_state.autofifo)
unlink(fifopath);
#endif
- if (g_state.pluginit)
- unlink(g_pipepath);
+ unlink(g_pipepath);
+ free(g_pipepath);
+ free(g_tmpfpath);
#ifdef DBGMODE
disabledbg();
#endif
@@ -7536,6 +7520,7 @@ int main(int argc, char *argv[])
#ifndef NORL
bool rlhist = FALSE;
#endif
+ size_t r;
while ((opt = (env_opts_id > 0
? env_opts[--env_opts_id]
@@ -7813,14 +7798,22 @@ int main(int argc, char *argv[])
DPRINTF_S(getenv("PWD"));
+ /* g_tmpfpath is used to generate tmp file names */
+ fd = getpid();
+ g_tmpfpath[tmpfplen - 1] = '\0';
+ g_pipepath = (char *)calloc(1, TMP_LEN_MAX);
+ if (!g_pipepath)
+ return EXIT_FAILURE;
+ r = mkpath(g_tmpfpath, "nnn-pipe.", g_pipepath);
+ xstrsncpy(g_pipepath + r - 1, xitoa(fd), TMP_LEN_MAX - r);
+ setenv(env_cfg[NNN_PIPE], g_pipepath, TRUE);
+
#ifndef NOFIFO
/* Create fifo */
if (g_state.autofifo) {
- g_tmpfpath[tmpfplen - 1] = '\0';
-
- size_t r = mkpath(g_tmpfpath, "nnn-fifo.", g_buf);
+ r = mkpath(g_tmpfpath, "nnn-fifo.", g_buf);
- xstrsncpy(g_buf + r - 1, xitoa(getpid()), PATH_MAX - r);
+ xstrsncpy(g_buf + r - 1, xitoa(fd), PATH_MAX - r);
setenv("NNN_FIFO", g_buf, TRUE);
}
If this doesn't work, we'll have to have someone work on it on his system. |
|
The above patch causes an immediate crash with a similar backtrace: In the interim, might be worth adding a note to the wiki that mac users might want to compile with the settings in the above comment: #786 (comment) if they run into problems. |
|
I think I understood the problem. The buffers are fine. It's the length 1024. As both the buffers are only 64 bytes they probably start close (maybe from the same page too) enough to be detected as overlapping. |
|
The following patch should work for you: iff --git a/src/nnn.c b/src/nnn.c
index 8328718..4b1bae5 100644
--- a/src/nnn.c
+++ b/src/nnn.c
@@ -1016,6 +1016,8 @@ static inline bool getutil(char *util)
/*
* Updates out with "dir/name or "/name"
* Returns the number of bytes copied including the terminating NULL byte
+ *
+ * Note: dir and out must be PATH_MAX in length to avoid macOS fault
*/
static size_t mkpath(const char *dir, const char *name, char *out)
{
@@ -4710,12 +4712,18 @@ static bool run_cmd_as_plugin(const char *file, char *runfile, uchar flags)
static bool plctrl_init(void)
{
- snprintf(g_buf, CMD_LEN_MAX, "nnn-pipe.%d", getpid());
+ size_t len;
+
/* g_tmpfpath is used to generate tmp file names */
g_tmpfpath[tmpfplen - 1] = '\0';
- mkpath(g_tmpfpath, g_buf, g_pipepath);
+ len = xstrsncpy(g_pipepath, g_tmpfpath, TMP_LEN_MAX);
+ g_pipepath[len - 1] = '/';
+ len = xstrsncpy(g_pipepath + len, "nnn-pipe.", TMP_LEN_MAX - len) + len;
+ xstrsncpy(g_pipepath + len - 1, xitoa(getpid()), TMP_LEN_MAX - len);
setenv(env_cfg[NNN_PIPE], g_pipepath, TRUE);
+ DPRINTF_S(g_pipepath);
+
return EXIT_SUCCESS;
}
|
|
For later reference, found a similar defect here. |
|
The issue is fixed in master now, thank you! |
|
No, thank you so much for reporting the issue! |
|
And running the tests. |
Environment details (Put
xin the checkbox along with the information)[x] Operating System: macOS Catalina 10.15.7
[ ] Desktop Environment:
[x] Terminal Emulator: iTerm2 (also occurs on Terminal)
[x] Shell: zsh 5.7.1 (also occurs on bash 5.0.18)
[ ] Custom desktop opener (if applicable):
[ ] Program options used:
[x] Configuration options set:
NNN_PLUG='i:ipinfo'NNN_FIFO='/tmp/nnn.fifo'[x] Issue exists on
nnnmasterExact steps to reproduce the issue
~/.config/nnn/pluginsNNN_PLUG.zsh: illegal hardware instruction ./nnnThis does not occur when installing nnn via homebrew. I initially discovered the issue when compiling with
O_NERD=1for the icons, but it occurs without any flags set as well.Debug output:
The text was updated successfully, but these errors were encountered: