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

fish 3.0 SIGSEGV under FreeBSD 12 #5453

Closed
mqudsi opened this Issue Dec 31, 2018 · 22 comments

Comments

Projects
None yet
3 participants
@mqudsi
Copy link
Contributor

mqudsi commented Dec 31, 2018

During the execution of the low level tests:

#0  0x00000008007bb26a in getpid () from /lib/libc.so.7
#1  0x00000000002f2a82 in is_forked_child () at ../src/common.cpp:2172
#2  assert_is_not_forked_child (who=0x203c62 "principal_parser") at ../src/common.cpp:2213
#3  0x000000000035f08e in parser_t::principal_parser () at ../src/parser.cpp:111
#4  0x0000000000372fe9 in job_t::promote (this=0x800a7fe58) at ../src/proc.cpp:128
#5  job_t::continue_job (this=0x800a7fe58, send_sigcont=false) at ../src/proc.cpp:1038
#6  0x000000000031ff1c in exec_job (parser=..., j=...) at ../src/exec.cpp:1084
#7  0x00000000003682cc in parse_execution_context_t::run_1_job (this=0x801d35d40, job_node=..., associated_block=<optimized out>) at ../src/parse_execution.cpp:1235
#8  0x00000000003640bc in parse_execution_context_t::run_job_conjunction (this=0x801d35d40, job_expr=..., associated_block=<optimized out>) at ../src/parse_execution.cpp:1274
#9  0x0000000000369021 in parse_execution_context_t::run_job_list<grammar::job_list> (this=0x801d35d40, job_list=..., associated_block=0x800a79f50) at ../src/parse_execution.cpp:1311
#10 0x0000000000365359 in parse_execution_context_t::run_while_statement (this=0x801d35d40, header=..., contents=..., associated_block=0x800a79f00) at ../src/parse_execution.cpp:555
#11 0x0000000000368178 in parse_execution_context_t::run_1_job (this=0x801d35d40, job_node=..., associated_block=0x800a79f00) at ../src/parse_execution.cpp:1153
#12 0x00000000003640bc in parse_execution_context_t::run_job_conjunction (this=0x801d35d40, job_expr=..., associated_block=<optimized out>) at ../src/parse_execution.cpp:1274
#13 0x0000000000369021 in parse_execution_context_t::run_job_list<grammar::job_list> (this=0x801d35d40, job_list=..., associated_block=0x800a79f00) at ../src/parse_execution.cpp:1311
#14 0x0000000000368a2d in parse_execution_context_t::eval_node (this=0x801d35d40, job_list=..., associated_block=0x800a79f00, io=...) at ../src/parse_execution.cpp:1359
#15 0x0000000000362044 in parser_t::eval_node<grammar::job_list> (this=0x39e9f0 <s_principal_parser>, ps=..., node=..., io=..., block_type=<optimized out>, parent_job=...) at ../src/parser.cpp:696
#16 0x00000000003612d3 in parser_t::eval (this=0x203c62, ps=..., io=..., block_type=54910) at ../src/parser.cpp:658
#17 0x0000000000360b5d in parser_t::eval (this=0x39e9f0 <s_principal_parser>, cmd=..., io=..., block_type=SUBST) at ../src/parser.cpp:648
#18 0x000000000032032e in exec_subshell_internal (cmd=..., lst=<optimized out>, apply_exit_status=true, is_subcmd=<optimized out>) at ../src/exec.cpp:1109
#19 0x0000000000324516 in expand_cmdsubst (input=..., out_list=0x7fffffff5810, errors=0x7fffffff5900) at ../src/expand.cpp:640
#20 0x0000000000322a49 in expand_stage_cmdsubst (input=..., out=0x7fffffff5810, flags=<optimized out>, errors=0x7fffffff5900) at ../src/expand.cpp:905
#21 0x0000000000322593 in expand_string (input=..., out_completions=0x7fffffff5930, flags=64, errors=0x7fffffff5900) at ../src/expand.cpp:1067
#22 0x0000000000364620 in parse_execution_context_t::expand_arguments_from_nodes (this=0x801d35bc0, argument_nodes=..., out_arguments=0x7fffffff59f0, glob_behavior=parse_execution_context_t::failglob) at ../src/parse_execution.cpp:883
#23 0x0000000000366b1f in parse_execution_context_t::populate_plain_process (this=<optimized out>, job=<optimized out>, proc=<optimized out>, statement=...) at ../src/parse_execution.cpp:844
#24 0x00000000003678aa in parse_execution_context_t::populate_job_from_job_node (this=0x801d35bc0, j=<optimized out>, job_node=..., associated_block=<optimized out>) at ../src/parse_execution.cpp:1071
#25 0x0000000000367fee in parse_execution_context_t::run_1_job (this=0x801d35bc0, job_node=..., associated_block=0x800a79c80) at ../src/parse_execution.cpp:1206
#26 0x00000000003640bc in parse_execution_context_t::run_job_conjunction (this=0x801d35bc0, job_expr=..., associated_block=<optimized out>) at ../src/parse_execution.cpp:1274
#27 0x0000000000369021 in parse_execution_context_t::run_job_list<grammar::job_list> (this=0x801d35bc0, job_list=..., associated_block=0x800a79c80) at ../src/parse_execution.cpp:1311
#28 0x0000000000368a2d in parse_execution_context_t::eval_node (this=0x801d35bc0, job_list=..., associated_block=0x800a79c80, io=...) at ../src/parse_execution.cpp:1359
#29 0x0000000000362044 in parser_t::eval_node<grammar::job_list> (this=0x39e9f0 <s_principal_parser>, ps=..., node=..., io=..., block_type=<optimized out>, parent_job=...) at ../src/parser.cpp:696
#30 0x00000000003612d3 in parser_t::eval (this=0x203c62, ps=..., io=..., block_type=54910) at ../src/parser.cpp:658
#31 0x0000000000360b5d in parser_t::eval (this=0x39e9f0 <s_principal_parser>, cmd=..., io=..., block_type=TOP) at ../src/parser.cpp:648
#32 0x00000000002ad0ba in test_1_cancellation (src=0x212740 L"echo (while true ; echo blah ; end)") at ../src/fish_tests.cpp:930
#33 0x000000000028ec99 in test_cancellation () at ../src/fish_tests.cpp:956
#34 main (argc=<optimized out>, argv=<optimized out>) at ../src/fish_tests.cpp:5008

It's an unhandled sigint in the call to getpid(). Not sure what to make of it.

This was against the fish 3.0 release, compiled under FreeBSD12-RELEASE x64 with clang 7.0.0

@mqudsi mqudsi changed the title fish 3.0 test failure under FreeBSD 12 fish 3.0 SIGINT under FreeBSD 12 in is_forked_child() Dec 31, 2018

@mqudsi

This comment has been minimized.

Copy link
Contributor Author

mqudsi commented Dec 31, 2018

This seems to be a memory corruption issue, because under master with my just-committed patches for is_forked_child() and is_main_thread() which don't call into the kernel at all, fish_tests still SIGINTs, this time with the following backtrace:

#0  std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >::size (this=<optimized out>) at /usr/include/c++/v1/string:896
#1  wcs2string (input=...) at ../src/common.cpp:370
#2  0x0000000000347c7b in separated_buffer_t<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >::append_wide_buffer<std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > > (this=0x800a78f70, rhs=...)
    at ../src/io.h:147
#3  io_buffer_t::append_from_stream (this=<optimized out>, stream=...) at ../src/io.cpp:40
#4  0x000000000031f763 in handle_builtin_output (p=<optimized out>, io_chain=0x800a7d150, j=..., builtin_io_streams=...) at ../src/exec.cpp:594
#5  exec_process_in_job (parser=..., p=<optimized out>, j=..., pipe_current_read=..., all_ios=..., stdout_read_limit=<optimized out>, out_pipe_next_read=<optimized out>) at ../src/exec.cpp:971
#6  exec_job (parser=..., j=...) at ../src/exec.cpp:1064
#7  0x00000000003681dc in parse_execution_context_t::run_1_job (this=0x801b41e60, job_node=..., associated_block=<optimized out>) at ../src/parse_execution.cpp:1238
#8  0x0000000000363fbc in parse_execution_context_t::run_job_conjunction (this=0x801b41e60, job_expr=..., associated_block=<optimized out>) at ../src/parse_execution.cpp:1277
#9  0x0000000000368f31 in parse_execution_context_t::run_job_list<grammar::job_list> (this=0x801b41e60, job_list=..., associated_block=0x800a79040) at ../src/parse_execution.cpp:1314
#10 0x0000000000365259 in parse_execution_context_t::run_while_statement (this=0x801b41e60, header=..., contents=..., associated_block=0x800a78ff0) at ../src/parse_execution.cpp:555
#11 0x0000000000368088 in parse_execution_context_t::run_1_job (this=0x801b41e60, job_node=..., associated_block=0x800a78ff0) at ../src/parse_execution.cpp:1156
#12 0x0000000000363fbc in parse_execution_context_t::run_job_conjunction (this=0x801b41e60, job_expr=..., associated_block=<optimized out>) at ../src/parse_execution.cpp:1277
#13 0x0000000000368f31 in parse_execution_context_t::run_job_list<grammar::job_list> (this=0x801b41e60, job_list=..., associated_block=0x800a78ff0) at ../src/parse_execution.cpp:1314
#14 0x000000000036893d in parse_execution_context_t::eval_node (this=0x801b41e60, job_list=..., associated_block=0x800a78ff0, io=...) at ../src/parse_execution.cpp:1362
#15 0x0000000000361f44 in parser_t::eval_node<grammar::job_list> (this=0x39d970 <s_principal_parser>, ps=..., node=..., io=..., block_type=<optimized out>, parent_job=...) at ../src/parser.cpp:695
#16 0x00000000003611d3 in parser_t::eval (this=0x7fffffff49e1, ps=..., io=..., block_type=4294920448) at ../src/parser.cpp:657
#17 0x0000000000360a5d in parser_t::eval (this=0x39d970 <s_principal_parser>, cmd=..., io=..., block_type=SUBST) at ../src/parser.cpp:647
#18 0x00000000003203ae in exec_subshell_internal (cmd=..., lst=<optimized out>, apply_exit_status=true, is_subcmd=<optimized out>) at ../src/exec.cpp:1109
#19 0x0000000000324556 in expand_cmdsubst (input=..., out_list=0x7fffffff5810, errors=0x7fffffff5900) at ../src/expand.cpp:640
#20 0x0000000000322ac9 in expand_stage_cmdsubst (input=..., out=0x7fffffff5810, flags=<optimized out>, errors=0x7fffffff5900) at ../src/expand.cpp:905
#21 0x0000000000322613 in expand_string (input=..., out_completions=0x7fffffff5930, flags=64, errors=0x7fffffff5900) at ../src/expand.cpp:1067
#22 0x0000000000364520 in parse_execution_context_t::expand_arguments_from_nodes (this=0x801b41ce0, argument_nodes=..., out_arguments=0x7fffffff59f0, glob_behavior=parse_execution_context_t::failglob) at ../src/parse_execution.cpp:886
#23 0x0000000000366a33 in parse_execution_context_t::populate_plain_process (this=<optimized out>, job=<optimized out>, proc=<optimized out>, statement=...) at ../src/parse_execution.cpp:847
#24 0x00000000003677ba in parse_execution_context_t::populate_job_from_job_node (this=0x801b41ce0, j=<optimized out>, job_node=..., associated_block=<optimized out>) at ../src/parse_execution.cpp:1074
#25 0x0000000000367efe in parse_execution_context_t::run_1_job (this=0x801b41ce0, job_node=..., associated_block=0x800a78640) at ../src/parse_execution.cpp:1209
#26 0x0000000000363fbc in parse_execution_context_t::run_job_conjunction (this=0x801b41ce0, job_expr=..., associated_block=<optimized out>) at ../src/parse_execution.cpp:1277
#27 0x0000000000368f31 in parse_execution_context_t::run_job_list<grammar::job_list> (this=0x801b41ce0, job_list=..., associated_block=0x800a78640) at ../src/parse_execution.cpp:1314
#28 0x000000000036893d in parse_execution_context_t::eval_node (this=0x801b41ce0, job_list=..., associated_block=0x800a78640, io=...) at ../src/parse_execution.cpp:1362
#29 0x0000000000361f44 in parser_t::eval_node<grammar::job_list> (this=0x39d970 <s_principal_parser>, ps=..., node=..., io=..., block_type=<optimized out>, parent_job=...) at ../src/parser.cpp:695
#30 0x00000000003611d3 in parser_t::eval (this=0x7fffffff49e1, ps=..., io=..., block_type=4294920448) at ../src/parser.cpp:657
#31 0x0000000000360a5d in parser_t::eval (this=0x39d970 <s_principal_parser>, cmd=..., io=..., block_type=TOP) at ../src/parser.cpp:647
#32 0x00000000002ad0ba in test_1_cancellation (src=0x2127f4 L"echo (while true ; echo blah ; end)") at ../src/fish_tests.cpp:930
#33 0x000000000028ec99 in test_cancellation () at ../src/fish_tests.cpp:956
#34 main (argc=<optimized out>, argv=<optimized out>) at ../src/fish_tests.cpp:5009

@mqudsi mqudsi changed the title fish 3.0 SIGINT under FreeBSD 12 in is_forked_child() fish 3.0 SIGINT under FreeBSD 12 Dec 31, 2018

@mqudsi

This comment has been minimized.

Copy link
Contributor Author

mqudsi commented Dec 31, 2018

Running it a few more times I ended up with some SIGSEGV, which agrees with the memory corruption hypothesis.

@mqudsi

This comment has been minimized.

Copy link
Contributor Author

mqudsi commented Dec 31, 2018

@faho it bisects to ffab420

ffab420e4318c65387a72a8572f2e40e301d0028 is the first bad commit
commit ffab420e4318c65387a72a8572f2e40e301d0028
Author: Fabian Homborg <FHomborg@gmail.com>
Date:   Wed Dec 12 15:12:12 2018 +0100

    Add fallback wcstod_l for musl

    Just sets locale to "C" (because that's the only one we need), does
    wcstod and resets the locale.

    No idea why uselocale(loc) failed for me, but it did.

    Fixes #5407.
@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 31, 2018

it bisects to ffab420

What is weird is that FreeBSD has wcstod_l. It builds without the fallback. So something must be off about our detection. Is it in a different header or something?

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 31, 2018

Yup, it's in xlocale apparently, instead of wchar.

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 31, 2018

@mqudsi: Do you have any idea how we could fix the cmake/configure check?

I haven't had my morning coffee yet, and I seem to be unable. E.g. in cmake/ConfigureChecks.cmake, changing

CHECK_CXX_SYMBOL_EXISTS(wcstod_l wchar.h HAVE_WCSTOD_L)

to

CHECK_CXX_SYMBOL_EXISTS(wcstod_l "xlocale.h;wchar.h" HAVE_WCSTOD_L)

doesn't seem to have the desired effect.

@mqudsi

This comment has been minimized.

Copy link
Contributor Author

mqudsi commented Dec 31, 2018

wchar.h is correct, xlocale.h is for wcstod, but wchar.h is for wcstod_l.

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 31, 2018

But... those calls both result in HAVE_WCSTOD_L being undefined.

Do we have to include both at the same time? The actual definition is in /usr/include/xlocale/_wchar.h.

@mqudsi

This comment has been minimized.

Copy link
Contributor Author

mqudsi commented Dec 31, 2018

Yes, that does appear to be the case:

Determining if the wcstod_l exist failed with the following output:
Change Dir: /usr/home/mqudsi/fish-shell/./build/CMakeFiles/CMakeTmp

Run Build Command:"/usr/local/bin/ninja" "cmTC_a4460"
[1/2] Building CXX object CMakeFiles/cmTC_a4460.dir/CheckSymbolExists.cxx.o
FAILED: CMakeFiles/cmTC_a4460.dir/CheckSymbolExists.cxx.o 
/home/mqudsi/clang+llvm-7.0.0-amd64-unknown-freebsd11/bin/clang++    -fdiagnostics-color=always -o CMakeFiles/cmTC_a4460.dir/CheckSymbolExists.cxx.o -c CheckSymbolExists.cxx
CheckSymbolExists.cxx:8:19: error: use of undeclared identifier 'wcstod_l'
  return ((int*)(&wcstod_l))[argc];
                  ^
1 error generated.
ninja: build stopped: subcommand failed.

File /usr/home/mqudsi/fish-shell/./build/CMakeFiles/CMakeTmp/CheckSymbolExists.cxx:
/* */
#include <wchar.h>

int main(int argc, char** argv)
{
  (void)argv;
#ifndef wcstod_l
  return ((int*)(&wcstod_l))[argc];
#else
  (void)argc;
  return 0;
#endif
}

but

Determining if the wcstod_l exist passed with the following output:
Change Dir: /usr/home/mqudsi/fish-shell/./build/CMakeFiles/CMakeTmp

Run Build Command:"/usr/local/bin/ninja" "cmTC_5f169"
[1/2] Building CXX object CMakeFiles/cmTC_5f169.dir/CheckSymbolExists.cxx.o
[2/2] Linking CXX executable cmTC_5f169

File /usr/home/mqudsi/fish-shell/./build/CMakeFiles/CMakeTmp/CheckSymbolExists.cxx:
/* */
#include <wchar.h>
#include <xlocale.h>

int main(int argc, char** argv)
{
  (void)argv;
#ifndef wcstod_l
  return ((int*)(&wcstod_l))[argc];
#else
  (void)argc;
  return 0;
#endif
}

But HAVE_WCSTOD_L remains undefined.

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 31, 2018

Ah. We're missing it in config.h, so we're always building with the fallback.

@mqudsi

This comment has been minimized.

Copy link
Contributor Author

mqudsi commented Dec 31, 2018

The actual check is broken. Adding a static_assert(false, ...); to fallback.cpp after #ifndef HAVE_WCSTOD_L fails under both FreeBSD and Linux. The difference is that under Linux it doesn't cause any problems.

Edit: you beat me to it.

faho added a commit that referenced this issue Dec 31, 2018

cmake: Add missing HAVE_WCSTOD_L #cmakedefine
Turns out we've been using the fallback everywhere.

See #5453.
@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 31, 2018

@mqudsi: Can you still reproduce this?

My FreeBSD 12 vm doesn't seem to anymore, with 7078aa4.

Note that this means either there's a bug in FreeBSD, or a bug in my "wcstod_l" (but not really wcstod_l because it ignores the passed locale and uses C instead).

In which case my money is on me being a bad C programmer.

@faho

This comment has been minimized.

Copy link
Member

faho commented Dec 31, 2018

Okay, this seems fixed on FreeBSD by not using the fallback, and the fallback works on alpine musl.

@faho faho closed this Dec 31, 2018

@faho faho added the bug label Dec 31, 2018

@faho faho added this to the fish-3.1 milestone Dec 31, 2018

@mqudsi mqudsi reopened this Dec 31, 2018

@mqudsi

This comment has been minimized.

Copy link
Contributor Author

mqudsi commented Dec 31, 2018

The problem is resolved by not using the fallback. The problem is that it's still using the fallback.

Can you compile with this change:

diff --git a/src/fallback.cpp b/src/fallback.cpp
index 91ed149e..ac2392b8 100644
--- a/src/fallback.cpp
+++ b/src/fallback.cpp
@@ -392,6 +392,7 @@ int flock(int fd, int op) {
 #ifndef HAVE_WCSTOD_L
 // musl doesn't feature wcstod_l,
 // so we just wrap wcstod.
+static_assert(false, "not working");
 double wcstod_l(const wchar_t *enptr, wchar_t **endptr, locale_t loc) {
     char *saved_locale = strdup(setlocale(LC_NUMERIC, NULL));
     // Yes, this is hardcoded to use the "C" locale.
@mqudsi

This comment has been minimized.

Copy link
Contributor Author

mqudsi commented Dec 31, 2018

Nevermind. I needed to delete the CMakeCache.txt file. I was doing that last night but forgot all about it today. Working now, thanks.

@mqudsi mqudsi closed this Dec 31, 2018

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jan 2, 2019

Fix `wcstod_l` infinite recursion under FreeBSD
This was the actual issue leading to memory corruption under FreeBSD in
issue fish-shell#5453, worked around by correcting the detection of `wcstod_l` so
that our version of the function is not called at all.

If we are 100% certain that `wcstod_l` does not exist, then then the
existing code is fine. But given that our checks have failed seperately
on two different platforms already (FreeBSD and Cygwin/newlib), it's a
good precaution to take.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jan 2, 2019

Fix `wcstod_l` infinite recursion under FreeBSD
This was the actual issue leading to memory corruption under FreeBSD in
issue fish-shell#5453, worked around by correcting the detection of `wcstod_l` so
that our version of the function is not called at all.

If we are 100% certain that `wcstod_l` does not exist, then then the
existing code is fine. But given that our checks have failed seperately
on two different platforms already (FreeBSD and Cygwin/newlib), it's a
good precaution to take.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jan 2, 2019

Fix `wcstod_l` infinite recursion under FreeBSD
This was the actual issue leading to memory corruption under FreeBSD in
issue fish-shell#5453, worked around by correcting the detection of `wcstod_l` so
that our version of the function is not called at all.

If we are 100% certain that `wcstod_l` does not exist, then then the
existing code is fine. But given that our checks have failed seperately
on two different platforms already (FreeBSD and Cygwin/newlib), it's a
good precaution to take.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Jan 3, 2019

Fix `wcstod_l` infinite recursion under FreeBSD
This was the actual issue leading to memory corruption under FreeBSD in
issue fish-shell#5453, worked around by correcting the detection of `wcstod_l` so
that our version of the function is not called at all.

If we are 100% certain that `wcstod_l` does not exist, then then the
existing code is fine. But given that our checks have failed seperately
on two different platforms already (FreeBSD and Cygwin/newlib), it's a
good precaution to take.
@mqudsi

This comment has been minimized.

Copy link
Contributor Author

mqudsi commented Jan 3, 2019

The underlying issue caught by the missing config.h define is that on some platforms (e.g. FreeBSD), strtod is defined via strtod_l, which meant that when we provided a local version of strtod_l that then called strtod, it would subsequently call our strtod_l override again, leading to an infinite recursion and exhaustion of the stack (somehow not caught by asan or valgrind).

Fixed as a precaution in bc0a0b4, as it was extremely hard to pin down what was going on and I pity anyone that would run into the issue in the future.

@zanchey zanchey referenced this issue Jan 12, 2019

Closed

fish 3.0.1? #5520

7 of 7 tasks complete

@mqudsi mqudsi changed the title fish 3.0 SIGINT under FreeBSD 12 fish 3.0 SIGSEGV under FreeBSD 12 Jan 17, 2019

@mqudsi mqudsi modified the milestones: fish 3.1.0, fish 3.0.1 Jan 17, 2019

@mqudsi

This comment has been minimized.

Copy link
Contributor Author

mqudsi commented Jan 17, 2019

I'm not going to edit the OP, but fwiw the fish 3.0 release is actively broken under FreeBSD - it isn't just the tests that are FUBAR'd here. Any code path that attempts to parse a string as a double will fail, e.g.

mqudsi@freebsd /u/h/m/fish-shell> printf "%f" 7.0
fish: './build/fish' terminated by signal SIGSEGV (Address boundary error)

uqs pushed a commit to freebsd/freebsd-ports that referenced this issue Jan 17, 2019

asomers
shells/fish: patch upstream issue #5453
This change fixes a segfault that would happen from operations like
'printf "%f" 7.0'.  Also, this change removes Python as a runtime
dependency.  That was supposed to have been done in r488840, but there was a
typo.

fish-shell/fish-shell#5453

Reported by:	Mahmoud Al-Qudsi <mqudsi@neosmart.net>
MFH:		2019Q1


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@490604 35697150-7ecd-e111-bb59-0022644237b5

uqs pushed a commit to freebsd/freebsd-ports that referenced this issue Jan 17, 2019

shells/fish: patch upstream issue #5453
This change fixes a segfault that would happen from operations like
'printf "%f" 7.0'.  Also, this change removes Python as a runtime
dependency.  That was supposed to have been done in r488840, but there was a
typo.

fish-shell/fish-shell#5453

Reported by:	Mahmoud Al-Qudsi <mqudsi@neosmart.net>
MFH:		2019Q1

swills pushed a commit to swills/freebsd-ports that referenced this issue Jan 17, 2019

shells/fish: patch upstream issue #5453
This change fixes a segfault that would happen from operations like
'printf "%f" 7.0'.  Also, this change removes Python as a runtime
dependency.  That was supposed to have been done in r488840, but there was a
typo.

fish-shell/fish-shell#5453

Reported by:	Mahmoud Al-Qudsi <mqudsi@neosmart.net>
MFH:		2019Q1


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@490604 35697150-7ecd-e111-bb59-0022644237b5

Jehops pushed a commit to Jehops/freebsd-ports that referenced this issue Jan 18, 2019

shells/fish: patch upstream issue #5453
This change fixes a segfault that would happen from operations like
'printf "%f" 7.0'.  Also, this change removes Python as a runtime
dependency.  That was supposed to have been done in r488840, but there was a
typo.

fish-shell/fish-shell#5453

Reported by:	Mahmoud Al-Qudsi <mqudsi@neosmart.net>
MFH:		2019Q1


git-svn-id: svn+ssh://svn.freebsd.org/ports/head@490604 35697150-7ecd-e111-bb59-0022644237b5

swills added a commit to swills/freebsd-ports that referenced this issue Jan 18, 2019

shells/fish: patch upstream issue #5453
This change fixes a segfault that would happen from operations like
'printf "%f" 7.0'.  Also, this change removes Python as a runtime
dependency.  That was supposed to have been done in r488840, but there was a
typo.

fish-shell/fish-shell#5453

Reported by:	Mahmoud Al-Qudsi <mqudsi@neosmart.net>
MFH:		2019Q1

mat813 pushed a commit to mat813/freebsd-ports that referenced this issue Jan 19, 2019

shells/fish: patch upstream issue #5453
This change fixes a segfault that would happen from operations like
'printf "%f" 7.0'.  Also, this change removes Python as a runtime
dependency.  That was supposed to have been done in r488840, but there was a
typo.

fish-shell/fish-shell#5453

Reported by:	Mahmoud Al-Qudsi <mqudsi@neosmart.net>
MFH:		2019Q1


git-svn-id: https://svn.freebsd.org/ports/head@490604 35697150-7ecd-e111-bb59-0022644237b5

zanchey added a commit that referenced this issue Jan 21, 2019

cmake: Add missing HAVE_WCSTOD_L #cmakedefine
Turns out we've been using the fallback everywhere.

See #5453.

(cherry picked from commit 7078aa4)
@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Jan 22, 2019

Reopening for 3.0.1

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Jan 23, 2019

@faho @mqudsi is 8ff8124 sufficient in Integration_3.0.1 for this? It seems to be in my test but I want to confirm.

mqudsi added a commit that referenced this issue Jan 23, 2019

Fix `wcstod_l` infinite recursion under FreeBSD
This was the actual issue leading to memory corruption under FreeBSD in
issue #5453, worked around by correcting the detection of `wcstod_l` so
that our version of the function is not called at all.

If we are 100% certain that `wcstod_l` does not exist, then then the
existing code is fine. But given that our checks have failed seperately
on two different platforms already (FreeBSD and Cygwin/newlib), it's a
good precaution to take.
@mqudsi

This comment has been minimized.

Copy link
Contributor Author

mqudsi commented Jan 23, 2019

No, FreeBSD needs bc0a0b4 to avoid the segfault and then there are a myriad of other commits that, combined with 8ff8124, prevent using the wcstod_l fallback when it's not needed (on pretty much all platforms).

I pushed the FreeBSD fix to the integration branch as a1df72d

If you want all the wcstod_l fixes, I think you'll need (in that order)

git cherry-pick a8eb02f9f59350f222295e16b8f68dc3b45d9ed5 74422e476b91643a6bea132bec78e580b6c45f2e 3444e1db18bcd1b9f7425a31895298c3864d294c 380bae80bf3e3b317b4b58bb73137831daace7e2 2bb53f7253765f652b21547a9aeb001b909aa05b f90cb3957f7c695b2ac0a32a848b2efcaa32ccaa c66b3128ec3d52897a2083b66574b8090bd11b1e

to get the following:

diff --git a/cmake/ConfigureChecks.cmake b/cmake/ConfigureChecks.cmake
index 1444361e..22b0066a 100644
--- a/cmake/ConfigureChecks.cmake
+++ b/cmake/ConfigureChecks.cmake
@@ -28,6 +28,7 @@ INCLUDE(CheckIncludeFiles)
 INCLUDE(CheckStructHasMember)
 INCLUDE(CheckCXXSourceCompiles)
 INCLUDE(CheckTypeSize)
+INCLUDE(CMakePushCheckState)
 CHECK_CXX_SYMBOL_EXISTS(backtrace_symbols execinfo.h HAVE_BACKTRACE_SYMBOLS)
 CHECK_CXX_SYMBOL_EXISTS(clock_gettime time.h HAVE_CLOCK_GETTIME)
 CHECK_CXX_SYMBOL_EXISTS(ctermid_r stdio.h HAVE_CTERMID_R)
@@ -72,7 +73,20 @@ CHECK_CXX_SYMBOL_EXISTS(wcsdup wchar.h HAVE_WCSDUP)
 CHECK_CXX_SYMBOL_EXISTS(wcslcpy wchar.h HAVE_WCSLCPY)
 CHECK_CXX_SYMBOL_EXISTS(wcsncasecmp wchar.h HAVE_WCSNCASECMP)
 CHECK_CXX_SYMBOL_EXISTS(wcsndup wchar.h HAVE_WCSNDUP)
-CHECK_CXX_SYMBOL_EXISTS(wcstod_l wchar.h HAVE_WCSTOD_L)
+
+CMAKE_PUSH_CHECK_STATE(RESET)
+# `wcstod_l` is a GNU-extension, sometimes hidden behind the following define
+LIST(APPEND CMAKE_REQUIRED_DEFINITIONS -D_GNU_SOURCE=1)
+# `xlocale.h` is required to find `wcstod_l` in `wchar.h` under FreeBSD, but
+# it's not present under Linux.
+SET(WCSTOD_L_INCLUDES "")
+CHECK_INCLUDE_FILES("xlocale.h" HAVE_XLOCALE_H)
+IF(HAVE_XLOCALE_H)
+    LIST(APPEND WCSTOD_L_INCLUDES "xlocale.h")
+ENDIF()
+LIST(APPEND WCSTOD_L_INCLUDES "wchar.h")
+CHECK_CXX_SYMBOL_EXISTS(wcstod_l "${WCSTOD_L_INCLUDES}" HAVE_WCSTOD_L)
+CMAKE_POP_CHECK_STATE()
 
 CHECK_CXX_SYMBOL_EXISTS(_sys_errs stdlib.h HAVE__SYS__ERRS)
 
diff --git a/config_cmake.h.in b/config_cmake.h.in
index 47cfcc5e..8071546f 100644
--- a/config_cmake.h.in
+++ b/config_cmake.h.in
@@ -153,6 +153,9 @@
 /* The size of wchar_t in bits. */
 #define WCHAR_T_BITS ${WCHAR_T_BITS}
 
+/* Define if xlocale.h is required for locale_t or wide character support */
+#cmakedefine HAVE_XLOCALE_H 1
+
 /* Enable large inode numbers on Mac OS X 10.5.  */
 #ifndef _DARWIN_USE_64_BIT_INODE
 # define _DARWIN_USE_64_BIT_INODE 1
diff --git a/osx/config.h b/osx/config.h
index d8036fee..233464b4 100644
--- a/osx/config.h
+++ b/osx/config.h
@@ -181,6 +181,9 @@
 /* Define to 1 if you have the `wcsndup' function. */
 /* #undef HAVE_WCSNDUP */
 
+/* Define to 1 if you have the `wcstod_l' function. */
+#define HAVE_WCSTOD_L 1
+
 /* Define to 1 if the winsize struct and TIOCGWINSZ macro exist */
 #define HAVE_WINSIZE 1
 
diff --git a/src/fallback.cpp b/src/fallback.cpp
index 8a7d6a0e..e3dadeae 100644
--- a/src/fallback.cpp
+++ b/src/fallback.cpp
@@ -389,18 +389,18 @@ int flock(int fd, int op) {
 
 #endif  // HAVE_FLOCK
 
-#ifndef HAVE_WCSTOD_L
+#if !defined(HAVE_WCSTOD_L) && !defined(__NetBSD__)
 #undef wcstod_l
 // For platforms without wcstod_l C extension, wrap wcstod after changing the
 // thread-specific locale.
 double fish_compat::wcstod_l(const wchar_t *enptr, wchar_t **endptr, locale_t loc) {
-    char *saved_locale = strdup(setlocale(LC_NUMERIC, NULL));
-    // Yes, this is hardcoded to use the "C" locale.
-    // That's the only thing we need, and uselocale(loc) broke in my testing.
-    setlocale(LC_NUMERIC, "C");
+    // Create and use a new, thread-specific locale
+    locale_t locale = newlocale(LC_NUMERIC, "C", nullptr);
+    locale_t prev_locale = uselocale(locale);
     double ret = wcstod(enptr, endptr);
-    setlocale(LC_NUMERIC, saved_locale);
-    free(saved_locale);
+    // Restore the old locale before freeing the locale we created and are still using
+    uselocale(prev_locale);
+    freelocale(locale);
     return ret;
 }
 #endif // defined(wcstod_l)
diff --git a/src/fallback.h b/src/fallback.h
index b935c846..4d78c46f 100644
--- a/src/fallback.h
+++ b/src/fallback.h
@@ -199,7 +199,10 @@ int flock(int fd, int op);
 
 #endif
 
-#ifndef HAVE_WCSTOD_L
+// NetBSD _has_ wcstod_l, but it's doing some weak linking hullabaloo that I don't get.
+// Since it doesn't have uselocale (yes, the standard function isn't there, the non-standard extension is),
+// we can't try to use the fallback.
+#if !defined(HAVE_WCSTOD_L) && !defined(__NetBSD__)
 // On some platforms if this is incorrectly detected and a system-defined
 // defined version of `wcstod_l` exists, calling `wcstod` from our own
 // `wcstod_l` can call back into `wcstod_l` causing infinite recursion.
diff --git a/src/wutil.h b/src/wutil.h
index 28a8540d..661e82d0 100644
--- a/src/wutil.h
+++ b/src/wutil.h
@@ -10,6 +10,10 @@
 #include <locale.h>
 #include <string>
 
+#ifdef HAVE_XLOCALE_H
+#include <xlocale.h>
+#endif
+
 #include "common.h"
 #include "maybe.h"

I'm 85% sure I remember that without these additional commits the build is broken under macOS 10.10 (Yosemite) and Cygwin.

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 23, 2019

+#if !defined(HAVE_WCSTOD_L) && !defined(NetBSD)

That NetBSD bit still confuses me, BTW. Because NetBSD defines wcstod_l behind these guards:

#if (_POSIX_C_SOURCE - 0) >= 200809L || defined(_NETBSD_SOURCE)

and AFAICT, we don't define any of them, so we shouldn't be getting it? Or at least we should also be detecting it in cmake?

But we can't use our fallback there because NetBSD does not have uselocale (despite that being POSIX).

Anyway, since we're not cherry-picking all the NetBSD fixes, c66b312 is useless for 3.0.1.

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 23, 2019

I have now picked a8eb02f 74422e4 3444e1d 380bae8 2bb53f7 f90cb39.

@faho faho closed this Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.