From: Paul Floyd Date: Sat, 21 Jan 2023 16:55:09 +0000 (+0100) Subject: Bug 464476 - Firefox fails to start under Valgrind X-Git-Tag: VALGRIND_3_21_0~214 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5d387642049bf366d913f572269f8bf16627ac10;p=thirdparty%2Fvalgrind.git Bug 464476 - Firefox fails to start under Valgrind On FreeBSD, Firefox uses the kern.proc.pathname.PID sysctl to get the binary path (where PID can be the actual pid or -1). The user path is /usr/local/bin/firefox which is a symlink to /usr/local/lib/firefox/firefox. This was failing because we were not handling this MIB. That meant that the sysctl returned the path for the binary of the running tool (e.g., /home/paulf/scratch/valgrind/memcheck/memcheck-amd64-freebsd). Firefox looks for files in the same directory. Since it was the wrong directory it failed to find them and exited. I also noticed a lot of _umtx_op errors. On analysis they are spurious. The wake ops take an "obj" argument, a pointer to a variable. They only use the address as a key for lookups and don't read the contents. --- diff --git a/.gitignore b/.gitignore index b422b173e2..2400b4571a 100644 --- a/.gitignore +++ b/.gitignore @@ -1354,6 +1354,7 @@ /memcheck/tests/freebsd/setproctitle /memcheck/tests/freebsd/sctp /memcheck/tests/freebsd/sctp2 +/memcheck/tests/freebsd/bug464476 # /memcheck/tests/amd64-freebsd /memcheck/tests/amd64-freebsd/*.stderr.diff diff --git a/NEWS b/NEWS index f2a380863b..c8ae4289cd 100644 --- a/NEWS +++ b/NEWS @@ -89,6 +89,7 @@ are not entered into bugzilla tend to get forgotten about or ignored. 460356 s390: Sqrt32Fx4 -- cannot reduce tree 462830 WARNING: unhandled amd64-freebsd syscall: 474 463027 broken check for MPX instruction support in assembler +464476 Firefox fails to start under Valgrind To see details of a given bug, visit https://bugs.kde.org/show_bug.cgi?id=XXXXXX diff --git a/coregrind/m_initimg/initimg-freebsd.c b/coregrind/m_initimg/initimg-freebsd.c index f0e2bcfd07..ad5452ed28 100644 --- a/coregrind/m_initimg/initimg-freebsd.c +++ b/coregrind/m_initimg/initimg-freebsd.c @@ -459,6 +459,7 @@ Addr setup_client_stack( void* init_sp, auxsize += sizeof(*cauxv); switch(cauxv->a_type) { case VKI_AT_EXECPATH: + // @todo PJF this is wrong this will be the name of the execed tool stringsize += VG_(strlen)(cauxv->u.a_ptr) + 1; break; case VKI_AT_CANARYLEN: @@ -685,6 +686,7 @@ Addr setup_client_stack( void* init_sp, break; case VKI_AT_EXECPATH: + // @todo PJF this is wrong this will be the name of the execed tool auxv->u.a_ptr = copy_str(&strtab, orig_auxv->u.a_ptr); break; case VKI_AT_CANARY: diff --git a/coregrind/m_libcfile.c b/coregrind/m_libcfile.c index 95fa4bce4f..65ed4aa486 100644 --- a/coregrind/m_libcfile.c +++ b/coregrind/m_libcfile.c @@ -562,8 +562,9 @@ SysRes VG_(stat) ( const HChar* file_name, struct vg_stat* vgbuf ) #else res = VG_(do_syscall2)(__NR_stat, (UWord)file_name, (UWord)&buf); #endif - if (!sr_isError(res)) + if (!sr_isError(res)) { TRANSLATE_TO_vg_stat(vgbuf, &buf); + } return res; } # else @@ -647,6 +648,26 @@ Int VG_(fstat) ( Int fd, struct vg_stat* vgbuf ) # endif } +#if defined(VGO_freebsd) +/* extend this to other OSes as and when needed */ +SysRes VG_(lstat) ( const HChar* file_name, struct vg_stat* vgbuf ) +{ + SysRes res; + VG_(memset)(vgbuf, 0, sizeof(*vgbuf)); + + struct vki_freebsd11_stat buf; +#if (FREEBSD_VERS >= FREEBSD_12) + res = VG_(do_syscall2)(__NR_freebsd11_lstat, (UWord)file_name, (UWord)&buf); +#else + res = VG_(do_syscall2)(__NR_lstat, (UWord)file_name, (UWord)&buf); +#endif + if (!sr_isError(res)) { + TRANSLATE_TO_vg_stat(vgbuf, &buf); + } + return res; +} +#endif + #undef TRANSLATE_TO_vg_stat #undef TRANSLATE_statx_TO_vg_stat @@ -1739,6 +1760,28 @@ const HChar *VG_(dirname)(const HChar *path) return buf; } +#if defined(VGO_freebsd) +#if (FREEBSD_VERS >= FREEBSD_13_0) +/* + * I did look at nicking this from FreeBSD, it's fairly easy to port + * but I was put off by the copyright and 3-clause licence + * Then I looked at nicking it from glibc but that is full of + * macros private functions and conditions for Windows. + * So I gave up as it is only for FreeBSD 11 and 12. + * + * It is somewhat hard-coded for sysctl_kern_proc_pathname + * and PRE(sys___sysctl) assuming resolved has + * VKI_PATH_MAX space. + */ +Bool VG_(realpath)(const HChar *path, HChar *resolved) +{ + vg_assert(path); + vg_assert(resolved); + return !sr_isError(VG_(do_syscall5)(__NR___realpathat, VKI_AT_FDCWD, (RegWord)path, (RegWord)resolved, VKI_PATH_MAX, 0)); +} +#endif +#endif + /*--------------------------------------------------------------------*/ /*--- end ---*/ diff --git a/coregrind/m_syswrap/syswrap-freebsd.c b/coregrind/m_syswrap/syswrap-freebsd.c index 1df874e735..b06dbe7e2b 100644 --- a/coregrind/m_syswrap/syswrap-freebsd.c +++ b/coregrind/m_syswrap/syswrap-freebsd.c @@ -55,6 +55,7 @@ #include "pub_core_syscall.h" #include "pub_core_syswrap.h" #include "pub_core_inner.h" +#include "pub_core_pathscan.h" #if defined(ENABLE_INNER_CLIENT_REQUEST) #include "pub_core_clreq.h" #endif @@ -1938,6 +1939,46 @@ static void sysctl_kern_usrstack(SizeT* out, SizeT* outlen) *outlen = sizeof(ULong); } +static Bool sysctl_kern_proc_pathname(HChar *out, SizeT *len) +{ + // is this stashed somewhere? + const HChar *exe_name = VG_(find_executable)(VG_(args_the_exename)); + +#if (FREEBSD_VERS >= FREEBSD_13_0) + return VG_(realpath)(exe_name, out); +#else + // poor man's realpath + const HChar *resolved_name; + HChar tmp[VKI_PATH_MAX]; + + struct vg_stat statbuf; + SysRes res = VG_(lstat)(exe_name, &statbuf); + + if (sr_isError(res)) { + return False; + } else if (VKI_S_ISLNK(statbuf.mode)) { + SizeT link_len = VG_(readlink)(exe_name, tmp, VKI_PATH_MAX); + tmp[link_len] = '\0'; + resolved_name = tmp; + } else { + // not a link + resolved_name = exe_name; + } + + if (resolved_name[0] != '/') { + // relative path + if (resolved_name[0] == '.' && resolved_name[1] == '/') { + resolved_name += 2; + } + VG_(snprintf)(out, *len, "%s/%s", VG_(get_startup_wd)(), resolved_name); + } else { + VG_(snprintf)(out, *len, "%s", resolved_name); + } + *len = VG_(strlen)(out)+1; + return True; +#endif +} + // SYS___sysctl 202 /* int __sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp, void *newp, size_t newlen); */ /* ARG1 ARG2 ARG3 ARG4 ARG5 ARG6 */ @@ -2003,7 +2044,7 @@ PRE(sys___sysctl) * saved to file static variables in that file, so we call * VG_(get_usrstack)() to retrieve them from there. */ - if (SARG2 >= 2 && ML_(safe_to_deref)(name, 2*sizeof(int))) { + if (SARG2 == 2 && ML_(safe_to_deref)(name, 2*sizeof(int))) { if (name[0] == 1 && name[1] == 33) { // kern.usrstack sysctl_kern_usrstack((SizeT*)ARG3, (SizeT*)ARG4); @@ -2014,7 +2055,7 @@ PRE(sys___sysctl) /* * 2. kern.ps_strings */ - if (SARG2 >= 2 && ML_(safe_to_deref)(name, 2*sizeof(int))) { + if (SARG2 == 2 && ML_(safe_to_deref)(name, 2*sizeof(int))) { if (name[0] == 1 && name[1] == 32) { if (sysctl_kern_ps_strings((SizeT*)ARG3, (SizeT*)ARG4)) { SET_STATUS_Success(0); @@ -2022,6 +2063,19 @@ PRE(sys___sysctl) } } + /* + * 3. kern.proc.pathname + */ + if (SARG2 == 4 && ML_(safe_to_deref)(name, 4*sizeof(int))) { + if (name[0] == 1 && name[1] == 14 && name[2] == 12) { + vki_pid_t pid = (vki_pid_t)name[3]; + if (pid == -1 || pid == VG_(getpid)()) { + sysctl_kern_proc_pathname((HChar *)ARG3, (SizeT *)ARG4); + SET_STATUS_Success(0); + } + } + } + PRE_REG_READ6(int, "__sysctl", int *, name, vki_u_int32_t, namelen, void *, oldp, vki_size_t *, oldlenp, void *, newp, vki_size_t, newlen); @@ -2039,12 +2093,19 @@ PRE(sys___sysctl) // 2. Both are non-NULL // this is a query of oldp, oldlenp will be read and oldp will // be written + // + // More thoughts on this + // if say oldp is a string buffer + // oldlenp will point to the length of the buffer + // + // but on return does oldlenp also get updated? // is oldlenp is not NULL, can write if (ARG4 != (UWord)NULL) { if (ARG3 != (UWord)NULL) { // case 2 above PRE_MEM_READ("sysctl(oldlenp)", (Addr)ARG4, sizeof(vki_size_t)); + PRE_MEM_WRITE("sysctl(oldlenp)", (Addr)ARG4, sizeof(vki_size_t)); if (ML_(safe_to_deref)((void*)(Addr)ARG4, sizeof(vki_size_t))) { PRE_MEM_WRITE("sysctl(oldp)", (Addr)ARG3, *(vki_size_t *)ARG4); } else { @@ -2063,7 +2124,7 @@ POST(sys___sysctl) { if (ARG4 != (UWord)NULL) { if (ARG3 != (UWord)NULL) { - //POST_MEM_WRITE((Addr)ARG4, sizeof(vki_size_t)); + POST_MEM_WRITE((Addr)ARG4, sizeof(vki_size_t)); POST_MEM_WRITE((Addr)ARG3, *(vki_size_t *)ARG4); } else { POST_MEM_WRITE((Addr)ARG4, sizeof(vki_size_t)); @@ -4303,8 +4364,9 @@ PRE(sys__umtx_op) case VKI_UMTX_OP_WAKE: PRINT( "sys__umtx_op ( %#" FMT_REGWORD "x, WAKE, %" FMT_REGWORD "u)", ARG1, ARG3); PRE_REG_READ3(long, "_umtx_op_wake", - struct umtx *, obj, int, op, unsigned long, val); - PRE_MEM_READ( "_umtx_op_wake(mtx)", ARG1, sizeof(struct vki_umtx) ); + vki_uintptr_t *, obj, int, op, int, val); + // PJF I don't think that the value of obj gets read, the address is being used as a key + //PRE_MEM_READ("_umtx_op_wake(obj)", ARG1, sizeof(vki_uintptr_t)); break; case VKI_UMTX_OP_MUTEX_TRYLOCK: PRINT( "sys__umtx_op ( %#" FMT_REGWORD "x, MUTEX_TRYLOCK, %" FMT_REGWORD "u, %#" FMT_REGWORD "x, %#" FMT_REGWORD "x)", ARG1, ARG3, ARG4, ARG5); @@ -4428,8 +4490,9 @@ PRE(sys__umtx_op) case VKI_UMTX_OP_WAKE_PRIVATE: PRINT( "sys__umtx_op ( %#" FMT_REGWORD "x, CV_WAKE_PRIVATE, %" FMT_REGWORD "u)", ARG1, ARG3); PRE_REG_READ3(long, "_umtx_op_wake_private", - struct umtx *, obj, int, op, unsigned long, id); - PRE_MEM_READ( "_umtx_op_wake_private(mtx)", ARG1, sizeof(struct vki_umtx) ); + vki_uintptr_t *, obj, int, op, int, val); + // PJF like OP_WAKE contents of obj not read + //PRE_MEM_READ("_umtx_op_wake_private(obj)", ARG1, sizeof(vki_uintptr_t)); break; case VKI_UMTX_OP_MUTEX_WAIT: // pthread_mutex_lock without prio flags @@ -4543,7 +4606,7 @@ POST(sys__umtx_op) case VKI_UMTX_OP_MUTEX_WAIT: /* Sets/clears contested bits */ case VKI_UMTX_OP_MUTEX_WAKE: /* Sets/clears contested bits */ if (SUCCESS) { - POST_MEM_WRITE( ARG1, sizeof(struct vki_umutex) ); + POST_MEM_WRITE( ARG1, sizeof(vki_uintptr_t) ); } break; case VKI_UMTX_OP_SET_CEILING: @@ -6430,6 +6493,9 @@ PRE(sys___sysctlbyname) SET_STATUS_Success(0); } + // @todo PJF kern.proc.pathname + // how is that done? jusr a pid or -1 in the string? + // read number of ints specified in ARG2 from mem pointed to by ARG1 PRE_MEM_READ("__sysctlbyname(name)", (Addr)ARG1, ARG2 * sizeof(int)); diff --git a/coregrind/pub_core_libcfile.h b/coregrind/pub_core_libcfile.h index 56289a494c..af1176ca92 100644 --- a/coregrind/pub_core_libcfile.h +++ b/coregrind/pub_core_libcfile.h @@ -110,6 +110,12 @@ extern Int VG_(mkstemp) ( const HChar* part_of_name, /*OUT*/HChar* fullname ); return if the working directory couldn't be found. */ extern void VG_(record_startup_wd) ( void ); +#if defined(VGO_freebsd) +#if (FREEBSD_VERS >= FREEBSD_13_0) +extern Bool VG_(realpath)(const HChar *path, HChar *resolved); +#endif +#endif + #endif // __PUB_CORE_LIBCFILE_H /*--------------------------------------------------------------------*/ diff --git a/include/pub_tool_libcfile.h b/include/pub_tool_libcfile.h index c42f1b8d4d..ff4c0d3a57 100644 --- a/include/pub_tool_libcfile.h +++ b/include/pub_tool_libcfile.h @@ -81,6 +81,9 @@ extern Int VG_(pipe) ( Int fd[2] ); extern Off64T VG_(lseek) ( Int fd, Off64T offset, Int whence ); extern SysRes VG_(stat) ( const HChar* file_name, struct vg_stat* buf ); +#if defined(VGO_freebsd) +extern SysRes VG_(lstat) ( const HChar* file_name, struct vg_stat* buf ); +#endif extern Int VG_(fstat) ( Int fd, struct vg_stat* buf ); extern SysRes VG_(dup) ( Int oldfd ); extern SysRes VG_(dup2) ( Int oldfd, Int newfd ); diff --git a/include/vki/vki-freebsd.h b/include/vki/vki-freebsd.h index 929eb74adb..a7344242e9 100644 --- a/include/vki/vki-freebsd.h +++ b/include/vki/vki-freebsd.h @@ -105,10 +105,10 @@ typedef vki_uint64_t __vki_fsblkcnt_t; typedef vki_uint64_t __vki_fsfilcnt_t; typedef vki_uint32_t __vki_gid_t; typedef vki_int64_t __vki_id_t; -typedef vki_uint32_t __vki_ino_t; +typedef vki_uint64_t __vki_ino_t; typedef vki_int32_t __vki_lwpid_t; typedef vki_uint16_t __vki_mode_t; -typedef vki_uint16_t __vki_nlink_t; +typedef vki_uint64_t __vki_nlink_t; typedef vki_int64_t __vki_off_t; typedef vki_int32_t __vki_pid_t; typedef vki_int64_t __vki_rlim_t; @@ -119,7 +119,7 @@ typedef vki_int32_t __vki_useconds_t; typedef __vki_ct_rune_t __vki_rune_t; typedef __vki_ct_rune_t __vki_wchar_t; typedef __vki_ct_rune_t __vki_wint_t; -typedef vki_uint32_t __vki_dev_t; +typedef vki_uint64_t __vki_dev_t; typedef vki_uint32_t __vki_fixpt_t; @@ -331,13 +331,13 @@ struct vki_tms { /* QQQ 4.x stat layout */ struct vki_freebsd11_stat { - vki_dev_t st_dev; - vki_ino_t st_ino; + vki_uint32_t st_dev; + vki_uint32_t st_ino; vki_mode_t st_mode; - vki_nlink_t st_nlink; + vki_uint16_t st_nlink; vki_uid_t st_uid; vki_gid_t st_gid; - vki_dev_t st_rdev; + vki_uint32_t st_rdev; #if 0 struct vki_timespec st_atimespec; struct vki_timespec st_mtimespec; @@ -376,19 +376,15 @@ unsigned int : */ struct vki_stat { - //vki_dev_t st_dev; - vki_uint64_t st_dev; - //vki_ino_t st_ino; - vki_uint64_t st_ino; - //vki_nlink_t st_nlink; - vki_uint64_t st_nlink; + vki_dev_t st_dev; + vki_ino_t st_ino; + vki_nlink_t st_nlink; vki_mode_t st_mode; vki_int16_t st_padding0; vki_uid_t st_uid; vki_gid_t st_gid; vki_int32_t st_padding1; - //vki_dev_t st_rdev; - vki_uint64_t st_rdev; + vki_dev_t st_rdev; #ifdef VKI_STAT_TIME_T_EXT vki_int32_t st_atim_ext; #endif diff --git a/memcheck/tests/freebsd/Makefile.am b/memcheck/tests/freebsd/Makefile.am index daf14f2f8e..5cf1eaf67e 100644 --- a/memcheck/tests/freebsd/Makefile.am +++ b/memcheck/tests/freebsd/Makefile.am @@ -90,7 +90,16 @@ EXTRA_DIST = \ sctp.stdout.exp \ sctp2.vgtest \ sctp2.stderr.exp \ - sctp2.stdout.exp + sctp2.stdout.exp \ + bug464476.vgtest \ + bug464476.stderr.exp \ + bug464476.stdout.exp \ + bug464476_abs_symlink.vgtest \ + bug464476_abs_symlink.stderr.exp \ + bug464476_abs_symlink.stdout.exp \ + bug464476_rel_symlink.vgtest \ + bug464476_rel_symlink.stderr.exp \ + bug464476_rel_symlink.stdout.exp check_PROGRAMS = \ statfs pdfork_pdkill getfsstat inlinfo inlinfo_nested.so extattr \ @@ -98,7 +107,7 @@ check_PROGRAMS = \ linkat scalar_fork scalar_thr_exit scalar_abort2 scalar_pdfork \ scalar_vfork stat file_locking_wait6 utimens access chmod_chown \ misc get_set_context utimes static_allocs fexecve errno_aligned_allocs \ - setproctitle sctp sctp2 + setproctitle sctp sctp2 bug464476 AM_CFLAGS += $(AM_FLAG_M3264_PRI) AM_CXXFLAGS += $(AM_FLAG_M3264_PRI) @@ -111,6 +120,8 @@ inlinfo_nested_so_SOURCES = inlinfo_nested.c inlinfo_nested_so_CFLAGS = $(AM_CFLAGS) -fPIC @FLAG_W_NO_UNINITIALIZED@ inlinfo_nested_so_LDFLAGS = -Wl,-rpath,$(top_builddir)/memcheck/tests/freebsd -shared -fPIC +bug464476_SOURCES = bug464476.cpp + if FREEBSD_VERS_13_PLUS check_PROGRAMS += realpathat scalar_13_plus eventfd1 eventfd2 diff --git a/memcheck/tests/freebsd/bug464476.cpp b/memcheck/tests/freebsd/bug464476.cpp new file mode 100644 index 0000000000..001ac21673 --- /dev/null +++ b/memcheck/tests/freebsd/bug464476.cpp @@ -0,0 +1,33 @@ +// roughly based on the code for Firefox class BinaryPath +// https://searchfox.org/mozilla-central/source/xpcom/build/BinaryPath.h#185 + +#include +#include +#include +#include +#include + +using std::cerr; +using std::cout; +using std::string; + +int main(int argc, char **argv) +{ + char aResult[PATH_MAX]; + int mib[4]; + mib[0] = CTL_KERN; + mib[1] = KERN_PROC; + mib[2] = KERN_PROC_PATHNAME; + mib[3] = -1; + + size_t len = PATH_MAX; + if (sysctl(mib, 4, aResult, &len, nullptr, 0) < 0) { + cerr << "sysctl failed\n"; + return -1; + } + if (string(aResult) == argv[1]) { + cout << "OK\n"; + } else { + cerr << "Not OK aResult " << aResult << " argv[1] " << argv[1] << '\n'; + } +} diff --git a/memcheck/tests/freebsd/bug464476.stderr.exp b/memcheck/tests/freebsd/bug464476.stderr.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/memcheck/tests/freebsd/bug464476.stdout.exp b/memcheck/tests/freebsd/bug464476.stdout.exp new file mode 100644 index 0000000000..d86bac9de5 --- /dev/null +++ b/memcheck/tests/freebsd/bug464476.stdout.exp @@ -0,0 +1 @@ +OK diff --git a/memcheck/tests/freebsd/bug464476.vgtest b/memcheck/tests/freebsd/bug464476.vgtest new file mode 100644 index 0000000000..b97d862e0c --- /dev/null +++ b/memcheck/tests/freebsd/bug464476.vgtest @@ -0,0 +1,3 @@ +prog: bug464476 +vgopts: -q +args: `pwd`/bug464476 diff --git a/memcheck/tests/freebsd/bug464476_abs_symlink.stderr.exp b/memcheck/tests/freebsd/bug464476_abs_symlink.stderr.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/memcheck/tests/freebsd/bug464476_abs_symlink.stdout.exp b/memcheck/tests/freebsd/bug464476_abs_symlink.stdout.exp new file mode 100644 index 0000000000..d86bac9de5 --- /dev/null +++ b/memcheck/tests/freebsd/bug464476_abs_symlink.stdout.exp @@ -0,0 +1 @@ +OK diff --git a/memcheck/tests/freebsd/bug464476_abs_symlink.vgtest b/memcheck/tests/freebsd/bug464476_abs_symlink.vgtest new file mode 100644 index 0000000000..29d6b80614 --- /dev/null +++ b/memcheck/tests/freebsd/bug464476_abs_symlink.vgtest @@ -0,0 +1,5 @@ +prereq: ln -s bug464476 `pwd`/bug464476_abs_symlink +prog: bug464476_abs_symlink +vgopts: -q +args: `pwd`/bug464476 +cleanup: rm bug464476_abs_symlink diff --git a/memcheck/tests/freebsd/bug464476_rel_symlink.stderr.exp b/memcheck/tests/freebsd/bug464476_rel_symlink.stderr.exp new file mode 100644 index 0000000000..e69de29bb2 diff --git a/memcheck/tests/freebsd/bug464476_rel_symlink.stdout.exp b/memcheck/tests/freebsd/bug464476_rel_symlink.stdout.exp new file mode 100644 index 0000000000..d86bac9de5 --- /dev/null +++ b/memcheck/tests/freebsd/bug464476_rel_symlink.stdout.exp @@ -0,0 +1 @@ +OK diff --git a/memcheck/tests/freebsd/bug464476_rel_symlink.vgtest b/memcheck/tests/freebsd/bug464476_rel_symlink.vgtest new file mode 100644 index 0000000000..c2eace73b1 --- /dev/null +++ b/memcheck/tests/freebsd/bug464476_rel_symlink.vgtest @@ -0,0 +1,5 @@ +prereq: ln -s bug464476 ./bug464476_rel_symlink +prog: bug464476_rel_symlink +vgopts: -q +args: `pwd`/bug464476 +cleanup: rm bug464476_rel_symlink