[Bug 258559] tcsh can crash in morecore() due to 32-bit arithmetic

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=258559

John Baldwin <jhb@FreeBSD.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jhb@FreeBSD.org
           Assignee|bugs@FreeBSD.org            |jhb@FreeBSD.org

--- Comment #1 from John Baldwin <jhb@FreeBSD.org> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.