From nobody Sun Jul 2 22:45:10 2023 X-Original-To: freebsd-hackers@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4QvPK33dvYz4lPq1 for ; Sun, 2 Jul 2023 22:45:27 +0000 (UTC) (envelope-from marklmi@yahoo.com) Received: from sonic311-24.consmr.mail.gq1.yahoo.com (sonic311-24.consmr.mail.gq1.yahoo.com [98.137.65.205]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4QvPK21gzjz3MZv for ; Sun, 2 Jul 2023 22:45:26 +0000 (UTC) (envelope-from marklmi@yahoo.com) Authentication-Results: mx1.freebsd.org; dkim=pass header.d=yahoo.com header.s=s2048 header.b="t/TDpBkc"; spf=pass (mx1.freebsd.org: domain of marklmi@yahoo.com designates 98.137.65.205 as permitted sender) smtp.mailfrom=marklmi@yahoo.com; dmarc=pass (policy=reject) header.from=yahoo.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1688337924; bh=YbQuR496XDMabU6/HsfZPTDP6awpqs60f2mOYoKWF+0=; h=From:Subject:Date:To:References:From:Subject:Reply-To; b=t/TDpBkcfdHaOEmDep7zUNZUUA0xFZXA5+190cgorGt0BKfuv3cJutSXSpIsinMmhMkssgl3YRJUYpB0kklaQNz07iBgH0P9f2bP0AT+wMSddb5aJyOx6bz5+gR7YZ728NwKFculmiNw19GQcFRzIjMzFXdQ1LivxxgcJ3eb3oXC0oS42dca3hVRF8wNoS6uXEuW7OJKI5uYZ1RxMJtBACskB+7kedVYA6o4NPL+iHtPYpl0fKZ6G/QQr5uLmGmccmerPKVHe2hc2P2fkoYvYztALHM+4jdpkCDh1CS3vEO4MFRc+JDjgP/hG7MpoyUrzN+63Pxv1GEWjFh4xQy8eQ== X-SONIC-DKIM-SIGN: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1688337924; bh=8gRDlrEnr1JndffV83u/k97j3APO7kMjSRAwbNNSMrp=; h=X-Sonic-MF:From:Subject:Date:To:From:Subject; b=P7QkOQlsaAiUpX1RrKSgKzfVe2xXmlgiMXuXNQnKtpM+8DsTFC5lM0EKxWDtb0CVIliOn71SWtLnyh0fswAg1GbKQq3dRTMmfo70MOS+ttuP/iXCJOC3TWu2/pXmLxsavPzKQnxAQYz01mHD5wOqix/De1o2hWbeG8aJ1udYzGl1Xjklo27B2rAdHeQeiCaIzjinKRmKbnymva15IQCgvUXC67QzyWmqFJD/xpx+yFRRIS+xpHQCM/5Nkvc3ssydJ6zONhRtK3bYKs4rbYVEnIrXrzx+LoBgQ0EDIRBc2AGFQitcURgrH3lqEjPiwPVM/98MMoSPCRok6GSMWaw+ug== X-YMail-OSG: 4OlZIJAVM1kEsSkzouu8217MsXoZOABzgocVFfCC8HRnIMKO1iUtQGXGErL6jzH eJeJn0HAt_juGCwYXy5XN5AyHB22NqL94W6c0aclMBOzN5xHkfXkM33JjwKJsKlme_ZkzT3DGqRN AYrAHeWNbEVvj0sfsHStQmB_1YTxpBGUZy5vKwcCsfu.hwkeKJ7T9j6LrpXskQv2.AlNgFThBLxn V_9bBe.mYyc5RVMajDXzTtmHAExy7xHHaFQVE4U_kF6xATWzcJJnDI3.EA7DIaUHnTVA6_YBxCwc qaHnB9DRY37QNMhECrAEJkXvE.kOwAlXMYrjQyj23bGOSyrtZOsbCTktpeoXJBOGhZPK3oHeqH34 WYWrRv4ZHLr0nfUxKZDv5wfVU6lyEMGSLgRj4iZI3AcdAhci3keYNQRffefPHsd8F1IiQfvNqnmO lNjpmJ238Z.XAwZ64_in0VwwiSbp59IOCwiWYMJQb2kJZ8TYGtNIdBwFCBRfZcFa3QPP0fa0imTd MazxTI0LQF3mW5AX5evdtLmVMfPTIBoeF8oWNv4qXi4wm04nDFs9XiOM4Am6bG4ytEGmDL0Gj_u9 yTqnGLcV9EY0Db5uWVqc5r5uLPlPOlfaLjh.p3.nRtEaU0Z_BTLLJJxEml_MDmfQqbczInLgL8sR Yi2V5KFKBWdmx6IJwOLMKtOwBdMCyBh5LkGqQKKNkLi64epXq8oRx3MHztaKFSNknIllorWoSMLd dz5CVymA7RCY9h2OkvwSsrXwGU_luuHKVjW6Xd5a_XWOGdU.2kPC_5acd7saEtMYos1sVxMl73OO L720ZJc3b6uWbx_AantBy4xPOA9nV.HOvbvm1vStOHofAY_WMne90sM1Fv7AsacW9OqxN9IhK8wC 9TOSaqnx3n4f6GfirUp5w.FIR3_O3.nH5tNbdhIyM3m5OrbRHL6WzNM0JmIy_sno_FdtpAQoGNP6 IkhfZ4QV7EbwzeX3GQXXY9vmJ0ldpG4Bk7acQ3G_LUbYF8XOS0xJ7Pg_HxyTu64f.pHbrYRUnAVM zFuX7cbUGYUUq_F4JUc5x4DXEx9AVgCYq.CEbK38huU_kpnJtXJtUPa8TFKhoP_KoTi0lImEaFVE 1XWRfcaHYQ.SxcxKLPlB9buvecwjywO7VTVQmqikFnwI7XD0YnviIgIbbv46G5B6Bqd8TKM8Yk6U 3sUYbv8V9hftWonVtar4RhbvECuYam9v7.WrTylkilySinBwwjslcI4hQPlwEdWzhhnl_0WMvdh4 hUaYIqmUjAMAiV5O1hZ.7jlmK2lgaZnAWFQMPp8bdfyVOkCrLlfKQO_riPXi8Zrfp.02XpO5oVZ9 AYzr6iynDarNRiFFEIlxhSvCwOf0THxlhlSnrc4qb7VHZz3g44oJfnvPu0033ZVxIowz4Nu5Kd5P kmuWCruj_Z8hASRx8sq06jEyvQ1GiPtscpUCZNxGKqfYC6VqX1C.._rLADtbpvBEydz9a7TsOZNJ s1CILX7CnFo5JrWidSMw4PK2khJqmbFOMD.d058TSVTpl9mmi7PzIeZQ9sgCVfeVbNs.TCWMb3xr TtODCttqn9XZOLTAz6MQYju3TFT5HNWNM88jK74Wl5yOombD38poMvdfb125HF0nNj4jmRqh7euW yZNFjRW8CltJiUIcjX10b5vKyzFk8NQH0_OtNHUsCo5pxPzPLYQr.Rrxt0Ux0tckbDVjfxVS4asc 9bG5SBRW02bnn_S2poWSsNOMagy1smctwAMaL7IbvutIqYt0bQ0xxXiONdqMT7zNoosG9VCxi7be faGNqDhc.CVSB_N24TDprzRMdpVE8Q.nGHDKnVpArZIWNf_733NpjGrm04WSigmHeUnV5hMQOj0o lFP2wB7z5qabI9cPGNIe0IHkjIMNXESG3bKI9YcrbO1lV.4PiUA6ykR31zt86Jc.2OOGnMXaYKz7 JqU4J9PpMz2exXGMHgWqqGPOPKqKyP3APRl1TDlpxER76q9.4sldxobDq1Z7IyhYXfzucBjnqvT8 414DmALH6PnKhS3jPbt1IT8gvdTQCa3xPryrE4e4PmXsSkyXSjvIEVZUEoy4x_Z15VwJ6L857E89 xhd59uqBzcoDWsLk1jtGT2NOZIb7wcD.BbthKQ5SJaCnOXyLip8e81PROOOE4fvymVR3JBfB33C. 2el..VQNvUPJLcfKVrmtwLleY7kU6eiPzHv9JOOpZRSw92FiSY_h5wSTFmMIcqfCbf4Rwn8.VBlG DSDR7zMjqSYQtl2UP8ED0Jv3lGtnitnJao9oH.sDIvMMxSd4afDhZkqS3VHZ3pEVhYtAjyGyuOJ0 - X-Sonic-MF: X-Sonic-ID: 7260f396-7eed-4438-b288-10e8862bac99 Received: from sonic.gate.mail.ne1.yahoo.com by sonic311.consmr.mail.gq1.yahoo.com with HTTP; Sun, 2 Jul 2023 22:45:24 +0000 Received: by hermes--production-bf1-5d96b4b9f-jv67c (Yahoo Inc. Hermes SMTP Server) with ESMTPA ID dc0e7531edaab47a77528a0b69c8f629; Sun, 02 Jul 2023 22:45:22 +0000 (UTC) From: Mark Millard Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit List-Id: Technical discussions relating to FreeBSD List-Archive: https://lists.freebsd.org/archives/freebsd-hackers List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-hackers@freebsd.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.600.7\)) Subject: Re: top vs. array sorted_state's out of bounds indexing (and related issues) Message-Id: Date: Sun, 2 Jul 2023 15:45:10 -0700 To: cliftonr@volcano.org, FreeBSD Hackers X-Mailer: Apple Mail (2.3731.600.7) References: X-Spamd-Result: default: False [1.38 / 15.00]; NEURAL_SPAM_MEDIUM(0.73)[0.734]; NEURAL_SPAM_LONG(0.60)[0.596]; NEURAL_SPAM_SHORT(0.55)[0.545]; MV_CASE(0.50)[]; DMARC_POLICY_ALLOW(-0.50)[yahoo.com,reject]; R_DKIM_ALLOW(-0.20)[yahoo.com:s=s2048]; R_SPF_ALLOW(-0.20)[+ptr:yahoo.com]; MIME_GOOD(-0.10)[text/plain]; ARC_NA(0.00)[]; FROM_HAS_DN(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DWL_DNSWL_NONE(0.00)[yahoo.com:dkim]; RCVD_IN_DNSWL_NONE(0.00)[98.137.65.205:from]; MID_RHS_MATCH_FROM(0.00)[]; RCVD_TLS_LAST(0.00)[]; ASN(0.00)[asn:36647, ipnet:98.137.64.0/20, country:US]; RCVD_COUNT_THREE(0.00)[3]; FREEMAIL_FROM(0.00)[yahoo.com]; TO_DN_SOME(0.00)[]; MLMMJ_DEST(0.00)[freebsd-hackers@freebsd.org]; DKIM_TRACE(0.00)[yahoo.com:+]; FROM_EQ_ENVFROM(0.00)[]; RCPT_COUNT_TWO(0.00)[2]; FREEMAIL_ENVFROM(0.00)[yahoo.com]; MIME_TRACE(0.00)[0:+]; RWL_MAILSPIKE_POSSIBLE(0.00)[98.137.65.205:from] X-Rspamd-Queue-Id: 4QvPK21gzjz3MZv X-Spamd-Bar: + X-ThisMailContainsUnwantedMimeParts: N Clifton Royston wrote on Date: Sun, 02 Jul 2023 21:44:10 UTC : > On 7/2/2023 9:52 AM, Clifton Royston wrote: > > On 6/19/2023 6:05 AM, Konstantin Belousov wrote: > >> On Sun, Jun 18, 2023 at 09:23:06PM -0700, Mark Millard wrote: > >>> usr.bin/top/machine.c (from main) has: > [...] > >>> > >>> static int sorted_state[] = { > >>> 0, /* not used */ > >>> 3, /* sleep */ > >>> 1, /* ABANDONED (WAIT) */ > >>> 6, /* run */ > >>> 5, /* start */ > >>> 2, /* zombie */ > >>> 4 /* stop */ > >>> }; > >>> > >>> Note the elements are 0..6, so 7 elements. > >>> > >>> It is accessed via code like: > >>> > >>> sorted_state[(unsigned char)(b)->ki_stat] > > [...] > >>> /* > >>> * These were process status values (p_stat), now they are only > used in > >>> * legacy conversion code. > >>> */ > >>> #define SIDL 1 /* Process being created by fork. */ > >>> #define SRUN 2 /* Currently runnable. */ > >>> #define SSLEEP 3 /* Sleeping on an address. */ > >>> #define SSTOP 4 /* Process debugging or suspension. */ > >>> #define SZOMB 5 /* Awaiting collection by parent. */ > >>> #define SWAIT 6 /* Waiting for interrupt. */ > >>> #define SLOCK 7 /* Blocked on a lock. */ > >>> > > [...] > >>> } else if (TD_ON_LOCK(td)) { > >>> kp->ki_stat = SLOCK; > > [...] > >> https://reviews.freebsd.org/D40607 > > > > I rarely comment here and hesitate to now, but out of curiosity I > > looked at the original report and at the chain of commits. > > > > It appears to me that in making the code more readable the final > > result may have accidentally inverted the sort order from the intended > > values (mostly.) A casual test wouldn't show this, as unless the sort > > order in top were changed, normally it would only come into play for > > two processes with equal CPU % and ticks which seems unlikely. > [...] > > And, following up to myself, I now realized that both the committed > patch *and* my reply completely missed addressing the original issue > Mark reported: > > No value for the index SLOCK is included in the array initializer, Not true now. See below. > so > SLOCK is still not translated to a meaningful index, and AFAICT it is > not included in the dimensions! SLOCK has a element in the array now. See below. > I expect it would still result in the > same out-of-bounds indexing problem Mark reported for that case. Nope. See below. > A further corrected comment and array initializer: > > /* > * proc_compare - comparison function for "qsort" > * Compares the resource consumption of two processes using five > * distinct keys. The keys (in descending order of importance) are: > * percent cpu, cpu ticks, state, resident set size, total virtual > * memory usage. The process states are ordered as follows (from most > * to least important): run, zombie, idle (being created), interrupt > * wait, blocked on lock, stop, sleep. > * The array declaration below maps a process state index into a > * number that reflects this ordering. > */ > > static const int sorted_state[] = { > [SRUN] = 7, /* Currently runnable. */ > [SZOMB] = 6, /* Awaiting collection by parent. */ > [SIDL] = 5, /* Process being created by fork. */ > [SWAIT] = 4, /* Waiting for interrupt. */ > [SLOCK] = 3, /* Blocked on a lock. */ > [SSTOP] = 2, /* Process debugging or suspension. */ > [SSLEEP] = 1, /* Sleeping on an address. */ > } [I ignore here specific choices/views of what sort order should result.] Looking at modern /usr/main-src/usr.bin/top/machine.c in main's source I see: static const int sorted_state[] = { [SIDL] = 3, /* being created */ [SRUN] = 1, /* running/runnable */ [SSLEEP] = 6, /* sleeping */ [SSTOP] = 5, /* stopped/suspended */ [SZOMB] = 2, /* zombie */ [SWAIT] = 4, /* intr */ [SLOCK] = 7, /* blocked on lock */ }; In other words, after substitutions for the macros: static const int sorted_state[] = { [1] = 3, /* being created */ [2] = 1, /* running/runnable */ [3] = 6, /* sleeping */ [4] = 5, /* stopped/suspended */ [5] = 2, /* zombie */ [6] = 4, /* intr */ [7] = 7, /* blocked on lock */ }; That notation means (being explicit about the size and the implicit [0] case): static const int sorted_state[8] = { [0] = 0, /* implicit case */ [1] = 3, /* being created */ [2] = 1, /* running/runnable */ [3] = 6, /* sleeping */ [4] = 5, /* stopped/suspended */ [5] = 2, /* zombie */ [6] = 4, /* intr */ [7] = 7, /* blocked on lock */ }; It spans all the indexes for use of: #define SIDL 1 /* Process being created by fork. */ #define SRUN 2 /* Currently runnable. */ #define SSLEEP 3 /* Sleeping on an address. */ #define SSTOP 4 /* Process debugging or suspension. */ #define SZOMB 5 /* Awaiting collection by parent. */ #define SWAIT 6 /* Waiting for interrupt. */ #define SLOCK 7 /* Blocked on a lock. */ So there is no out of bounds access for any of those named values. === Mark Millard marklmi at yahoo.com