NetBSD Problem Report #58717

From www@netbsd.org  Sat Oct  5 08:52:20 2024
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id E80281A923D
	for <gnats-bugs@gnats.NetBSD.org>; Sat,  5 Oct 2024 08:52:19 +0000 (UTC)
Message-Id: <20241005085218.6D87B1A923E@mollari.NetBSD.org>
Date: Sat,  5 Oct 2024 08:52:18 +0000 (UTC)
From: cryintothebluesky@gmail.com
Reply-To: cryintothebluesky@gmail.com
To: gnats-bugs@NetBSD.org
Subject: wm/cde crashes in ttsession during startup on sparc64
X-Send-Pr-Version: www-1.0

>Number:         58717
>Category:       pkg
>Synopsis:       wm/cde crashes in ttsession during startup on sparc64
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    pkg-manager
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sat Oct 05 08:55:00 +0000 2024
>Last-Modified:  Sat Oct 05 12:30:02 +0000 2024
>Originator:     Sad Clouds
>Release:        pkg-2024Q2
>Organization:
>Environment:
>Description:
Pkgsrc wm/cde desktop has a number of issues on sparc64 and other architectures with LP64 data model.

The issue described in this bug occurs with ttsession executable, which crashes with SIGBUS due to alignment problems, where the original programmer assumed size of integer == size of long.
>How-To-Repeat:
Build wm/cde on sparc64 and then run /usr/pkg/bin/startcde
>Fix:
The patch below assumes that uid_t, gid_t, mode_t can be safely cast to unsigned int without data loss. Most platforms seem to define these types as either signed or unsigned 32-bit integer, so should be OK.

There are other executables like dthelpview which crash with SIGBUS, but at least with this patch CDE can be usable on sparc64.

--- cde-2.5.2/lib/tt/lib/db/db_server_xdr.C.orig	2023-11-18 22:38:09.000000000 +0000
+++ cde-2.5.2/lib/tt/lib/db/db_server_xdr.C	2024-10-05 06:59:06.723430670 +0100
@@ -108,7 +108,7 @@
 			 (xdrproc_t) xdr_keydesc)) {
 		return (FALSE);
 	}
-	if (!xdr_int(xdrs, &objp->mode)) {
+	if (!xdr_u_int(xdrs, (unsigned *)&objp->mode)) {
 		return (FALSE);
 	}
 	if (!xdr_int(xdrs, &objp->isreclen)) {
@@ -168,7 +168,7 @@
 	if (!xdr_string(xdrs, &objp->path, 1024)) {
 		return (FALSE);
 	}
-	if (!xdr_int(xdrs, &objp->mode)) {
+	if (!xdr_u_int(xdrs, (unsigned *)&objp->mode)) {
 		return (FALSE);
 	}
 	return (TRUE);
@@ -183,7 +183,7 @@
 	if (!xdr_array(xdrs, (char **)&objp->rec.rec_val, (u_int *)&objp->rec.rec_len, ISMAXRECLEN, sizeof(char), (xdrproc_t) xdr_char)) {
 		return (FALSE);
 	}
-	if (!xdr_int(xdrs, &objp->mode)) {
+	if (!xdr_u_int(xdrs, (unsigned *)&objp->mode)) {
 		return (FALSE);
 	}
 	if (!xdr_long(xdrs, &objp->isrecnum)) {
@@ -242,7 +242,7 @@
 	if (!xdr_array(xdrs, (char **)&objp->rec.rec_val, (u_int *)&objp->rec.rec_len, ISMAXRECLEN, sizeof(char), (xdrproc_t)xdr_char)) {
 		return (FALSE);
 	}
-	if (!xdr_int(xdrs, &objp->mode)) {
+	if (!xdr_u_int(xdrs, (unsigned *)&objp->mode)) {
 		return (FALSE);
 	}
 	return (TRUE);
@@ -380,13 +380,13 @@
 bool_t
 xdr_Tt_oidaccess_results(XDR *xdrs, _Tt_oidaccess_results *objp)
 {
-	if (!xdr_long(xdrs, (long *)&objp->uid)) {
+	if (!xdr_u_int(xdrs, (unsigned *)&objp->uid)) {
 		return (FALSE);
 	}
-	if (!xdr_long(xdrs, (long *)&objp->group)) {
+	if (!xdr_u_int(xdrs, (unsigned *)&objp->group)) {
 		return (FALSE);
 	}
-	if (!xdr_short(xdrs, (short *)&objp->mode)) {
+	if (!xdr_u_int(xdrs, (unsigned *)&objp->mode)) {
 		return (FALSE);
 	}
 	if (!xdr_int(xdrs, &objp->result)) {
@@ -503,13 +503,13 @@
 bool_t
 xdr_tt_access(XDR *xdrs, _tt_access *objp)
 {
-	if (!xdr_long(xdrs, (long *)&objp->user)) {
+	if (!xdr_u_int(xdrs, (unsigned *)&objp->user)) {
 		return (FALSE);
 	}
-	if (!xdr_long(xdrs, (long *)&objp->group)) {
+	if (!xdr_u_int(xdrs, (unsigned *)&objp->group)) {
 		return (FALSE);
 	}
-	if (!xdr_u_long(xdrs, (unsigned long *)&objp->mode)) {
+	if (!xdr_u_int(xdrs, (unsigned *)&objp->mode)) {
 		return (FALSE);
 	}
 	return (TRUE);

>Audit-Trail:
From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: pkg/58717: wm/cde crashes in ttsession during startup on sparc64
Date: Sat, 5 Oct 2024 12:03:16 +0200

 This needs more explanation and should be filed as an upstream bug report
 and be resolved properly.

 You seem to patch mostly signed int vs unsigned int issues, which should not
 matter at this level. I would even silence any compiler warnings about
 mismatches via -Wno-.... options.

 Then there are a few xdr_long -> xdr_u_int translations which probably
 prevent the crashes you talked about, but it is hard to tell without
 more context if those are correct. If the long was an error and the
 value actually is int (or unsigned int) those patches would be OK, but
 if the real value is long you now only encode half of the data - either
 the lower or upper half, and it might break _LP64 little endian
 architectures.

 But the original code already casting the pointer is a red flag - you
 basically *never* should cast in this context, or something else is
 very wrong.

 This code is ugly, and the real way to fix it probably is to create 
 xdr_* helper functions to use for all the relevant types, like:

 static bool_t
 xdr_uid(XDR *xdrs, cost uid_t *uid)
 {
 	if (sizeof(*uid) == sizeof(long))
 		xdr_long(xdrs, uid);
 	else if (sizeof(*uid) == sizeof(int))
 		xdr_int(xdrs, uid);
 	else
 		assert(false);
 }

 Would be a nice use for C++ templates :-)
 You could hide this in macros, but that obfuscates it even more.

 Martin

From: Sad Clouds <cryintothebluesky@gmail.com>
To: gnats-bugs@netbsd.org
Cc: "Martin Husemann via gnats" <gnats-admin@NetBSD.org>,
 pkg-manager@netbsd.org, gnats-admin@netbsd.org, pkgsrc-bugs@netbsd.org
Subject: Re: pkg/58717: wm/cde crashes in ttsession during startup on
 sparc64
Date: Sat, 5 Oct 2024 13:12:40 +0100

 I think originally, many Unix systems used long integer for uid_t and
 gid_t, however this results in different size on LP64 systems. See
 https://docs.oracle.com/cd/E18752_01/html/816-5138/appendixa-1.html

 Ideally they should have used fixed width integers, but the original
 implementation probably predates that version of C standard. I'm not
 aware of any platforms using full 64-bits for uid_t and gid_t. So even
 if those are defined as 64-bit integers, they should always contain
 values <= UINT32_MAX. So casting this from 64-bit to 32-bit should be
 safe and should not result in data loss. Now if this assumption is
 wrong, then using sizeof() for each type would be a better solution.

 I'm not sure there is a standard definition for uid_t, gid_t and
 mode_t. So these could be defined as signed or unsigned. Also I'm not
 sure if there is a difference in behavior between xdr_int() and
 xdr_u_int(), but it feels that using the unsigned conversion is
 probably safer and may avoid sign extension bugs.

From: Sad Clouds <cryintothebluesky@gmail.com>
To: gnats-bugs@netbsd.org
Cc: "Martin Husemann via gnats" <gnats-admin@NetBSD.org>,
 pkg-manager@netbsd.org, gnats-admin@netbsd.org, pkgsrc-bugs@netbsd.org
Subject: Re: pkg/58717: wm/cde crashes in ttsession during startup on
 sparc64
Date: Sat, 5 Oct 2024 13:26:57 +0100

 There is already a bug for this issue upstream which was created by
 somebody about a year ago:
 https://sourceforge.net/p/cdesktopenv/tickets/153/

 One of the developers used Qemu to simulate sparc64, but then if the
 emulator runs on x86 and allows unaligned loads/stores, then they would
 not have reproduced the exact issue.

Home	PR Database Search