Bug 258559
| Summary: | tcsh can crash in morecore() due to 32-bit arithmetic | ||
|---|---|---|---|
| Product: | Base System | Reporter: | Robert Morris <rtm> |
| Component: | bin | Assignee: | John Baldwin <jhb> |
| Status: | Open --- | ||
| Severity: | Affects Only Me | CC: | dchagin, emaste, jhb |
| Priority: | --- | ||
| Version: | 13.0-RELEASE | ||
| Hardware: | Any | ||
| OS: | Any | ||
Hmm, for me I don't get a crash but instead the system eventually runs out of swap. This code is indeed buggy and fraught with peril, but it also uses sbrk() which is a deprecated interface on FreeBSD (it's not even present in newer architectures: arm64 and riscv64). [t]csh from the base system should be built with SYSMALLOC defined so that tcsh uses the malloc() from libc instead of the version from tc.alloc.c. You can verify this by checking for the string ",sm" in the $version variable from tcsh. For example: > echo $version tcsh 6.22.04 (Astron) 2021-04-26 (x86_64-amd-FreeBSD) options wide,nls,dl,al,kan,sm,rh,color,filec Checking in a 12.x jail I had lying around shows ",sm" in $version as well, and the most recent change to bin/csh/config.h related to this was made in 2007: commit 59dfb2db03584859a57ab6d9519e2de147bf3c4d Author: Mark Peek <mp@FreeBSD.org> Date: Wed May 16 21:22:38 2007 +0000 Work around a vendor issue that was causing the builtin malloc to be used instead of the system malloc. Submitted by: ume Notes: svn path=/head/; revision=169626 diff --git a/bin/csh/config.h b/bin/csh/config.h index c9b01ef7aa94..0971ffa3faa1 100644 --- a/bin/csh/config.h +++ b/bin/csh/config.h @@ -237,3 +237,6 @@ #ifndef NO_NLS_CATALOGS #define NLS_CATALOGS #endif + +/* Work around a vendor issue where config_f.h is #undef'ing this setting */ +#define SYSMALLOC diff --git a/bin/csh/config_p.h b/bin/csh/config_p.h index 6de288b387e5..8c29053e3176 100644 --- a/bin/csh/config_p.h +++ b/bin/csh/config_p.h @@ -82,8 +82,6 @@ #if defined(__FreeBSD__) #define NLS_BUGS #define BSD_STYLE_COLORLS -/* we want to use the system malloc when we install as /bin/csh */ -#define SYSMALLOC /* Use LC_MESSAGES locale category to open the message catalog */ #define MCLoadBySet NL_CAT_LOCALE #define BUFSIZE 8192 So I'm surprised you would have a tcsh binary on a 13.0 system that was using the malloc() implementation from tc.alloc.c rather than the libc allocator. I do think it's true that if you checkout the raw tcsh sources from upstream and build them that it still defaults to using tc.alloc.c. If that is the way you built tcsh then a patch to tcsh itself to adjust these lines in the upstream config_f.h would work: /* * SYSMALLOC Use the system provided version of malloc and friends. * This can be much slower and no memory statistics will be * provided. */ #if defined(__MACHTEN__) || defined(PURIFY) || defined(MALLOC_TRACE) || defined(_OSD_POSIX) || defined(__MVS__) || defined (__CYGWIN__) || defined(__GLIBC__) || defined(__OpenBSD__) || defined(__APPLE__) || defined (__ANDROID__) # define SYSMALLOC #else # undef SYSMALLOC #endif IMO, upstream tcsh would be better served by just always assuming SYSMALLOC and dropping its own allocator as it is unlikely to be better than the system malloc on any contemporary OSs. |
tcsh's allocator uses 32-bit ints in pointer arithmetic in a way that's buggy on 64-bit machines: in morecore() in tc.alloc.c, if bucket is >= 28, then these lines aren't right: memtop = sbrk(1 << rnu); ... memtop += (long) (1 << rnu); ... siz = 1 << (bucket + 3); The shifts yield 32-bit signed values, but the amounts of shift can be big enough that the results wrap. Here's a way to see the crash (it takes a few tens of seconds): % echo '$x:s<' | tcsh -f