From owner-svn-src-head@freebsd.org Wed Jan 24 18:05:26 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id BA9E1EB9262 for ; Wed, 24 Jan 2018 18:05:26 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-wm0-x22f.google.com (mail-wm0-x22f.google.com [IPv6:2a00:1450:400c:c09::22f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 2A1AE6CFC4 for ; Wed, 24 Jan 2018 18:05:26 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-wm0-x22f.google.com with SMTP id 141so10243851wme.3 for ; Wed, 24 Jan 2018 10:05:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=8EZp464fjPIQhNjh2ltYHDS58c1nRjG9zPEZNztGFZk=; b=SDbBt57Rxt/84a/wRSivCsRQ/WXh/vrzCSj1uz6L81fEoSNxyBlk68AjjFEs8W2cKV C6n+PvssCKJ1HgSlPY3o+AETlE6SJSFB6hRG6pBdQL6yHO4nxr9R+R4lcq3bJSS/s82G o729NyEKOdqc9MvF9A1d6sJxqhMwBT5SfYO96M/h/6DII8y6ta+evtmdgfKG35cn40kz U4az2UeoQzu1tPvbw4dSX74oY9o16WwXSPBat41uVHRrLQ0D4AX2tswm5XApSaRdQXdE XUlmefCK9n405RjngTy/1+V/+VhFSSXQFuNpFJPHrUjZnnlD6nnQT0crh74w5m1/LY/n EIcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=8EZp464fjPIQhNjh2ltYHDS58c1nRjG9zPEZNztGFZk=; b=ZEM/82G8PLEY3FHCJNyquot1GZuPsLfQzjXfKtqIwDR9PPcieTOQwF+/QmqAdm0sFT AsWn+bdYY/xarpKXtLAaYswRP4YQYwdpVlAG09YpBO8v7W0p6VCh7BzxpTlhM7B0kjgc ryocN5qKNkIaPKopfaz9ygvkRykh8OL0B3suUROhXgQ052nOCLGPOghSVWKu1NRapc/K OXREmeU7vU2fOhhtIUnafdnBdmty6ZrIDLU04QcHNGiK4Jqog+JV+DZhr1eQP3X+Yz6e 7alxxw+YaPFGFUoNMlztanlGwEjbD/P+vYRwVij3chQTs17idSRtQaa+W+IRQliZLAbd nIkQ== X-Gm-Message-State: AKwxytd+RkXagvHCLhYGT+1PyOcHIn4w5kXvoDcmC+rHsm/L/GjOachm Y15KiLld42ikxC8rFLVExDjYKLYCDogTnO7r5BFNjw== X-Google-Smtp-Source: AH8x224HdSZsicKS7KA4nKKHC2MrM5NqWuHzMAJAYFan/+JlkLbSbSNOyJs6ojVyB48sbEPgisf3Tw/HYgTkrf2b+1k= X-Received: by 10.80.217.202 with SMTP id x10mr19908343edj.118.1516817124970; Wed, 24 Jan 2018 10:05:24 -0800 (PST) MIME-Version: 1.0 Sender: wlosh@bsdimp.com Received: by 10.80.133.195 with HTTP; Wed, 24 Jan 2018 10:05:24 -0800 (PST) X-Originating-IP: [50.227.106.226] In-Reply-To: References: <201801211542.w0LFgbsp005980@repo.freebsd.org> <51ff8aef-5660-7857-e4d5-12cdc77bc071@FreeBSD.org> <20180124182548.X1063@besplex.bde.org> From: Warner Losh Date: Wed, 24 Jan 2018 11:05:24 -0700 X-Google-Sender-Auth: 9q-vpAnnZFlwEgvdvWGIqRoXT18 Message-ID: Subject: Re: svn commit: r328218 - in head/sys: amd64/amd64 arm/xscale/ixp425 arm64/arm64 cam cam/ctl compat/ndis dev/aacraid dev/advansys dev/ath dev/beri/virtio dev/bnxt dev/bwn dev/ciss dev/cxgbe/crypto dev/... To: "Conrad E. Meyer" Cc: src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.25 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Jan 2018 18:05:27 -0000 On Wed, Jan 24, 2018 at 10:34 AM, Conrad Meyer wrote: > On Wed, Jan 24, 2018 at 7:44 AM, Warner Losh wrote: > > I agree completely. It doesn't do what you think it is doing, for all t= he > > reasons that Bruce outlines. We thought it was a bad idea when it came > up 2 > > years ago and nothing has really changed. > > I disagree. I'm not sure what you mean by "it doesn't do what you > think it is doing." Do you think the manual page is unclear or needs > more detail? It seems clear to me, but it also does what I think it > does. > It changes the fundamental 'can't fail' allocations into 'maybe panic' allocations, which was my big objection. > Your description of two years ago is inaccurate =E2=80=94 you thought it = was a > bad idea, and were the most vocal on the mailing list about it, but > that viewpoint was not universally shared. In a pure headcount vote I > think you were even outvoted, but as the initiative was headed by a > non-committer, it sputtered out. > I don't recall that happening. But regardless, mallocarray, as implemented today, is useless. > If Bruce has made some important point or illumination, please > highlight it. It's buried in the mostly nonsense wall of text > boilerplate he usually includes. > Let's start with his point about u_long vs size_t causing problems: void *malloc(unsigned long size, struct malloc_type *type, int flags) vs void *mallocarray(size_t nmemb, size_t size, struct malloc_type *type, Since size_t is long long on i386, for example, this can result in undetected overflows. Such inattention to detail makes me extremely uneasy about the rest of the code. mallocarray serves an important function =E2=80=94 a last ditch seatbelt > against overflowing allocations that can trivially replace existing > naive malloc calls containing multiplication. Trivial heap corruption > is replaced with DoS =E2=80=94 a strict improvement. That is all it does= . > It's an important function, but it's so poorly implement we should try again. It is not safe nor wise to use it blindly, which was bde's larger point. #define MUL_NO_OVERFLOW (1UL << (sizeof(size_t) * 8 / 2)) static inline bool WOULD_OVERFLOW(size_t nmemb, size_t size) { return ((nmemb >=3D MUL_NO_OVERFLOW || size >=3D MUL_NO_OVERFLOW) &= & nmemb > 0 && __SIZE_T_MAX / nmemb < size); } So if I pass in 1GB and 10, this will tell me it won't overflow because of size_t can handle 10GB, but u_long can't. On amd64, it won't, but on i386 it will since long is 32 bits leading to issues in this code: void * mallocarray(size_t nmemb, size_t size, struct malloc_type *type, int flags) { if (WOULD_OVERFLOW(nmemb, size)) panic("mallocarray: %zu * %zu overflowed", nmemb, size); return (malloc(size * nmemb, type, flags)); } since malloc takes u_long, size * nmemb would overflow yielding bad results= . Many places that use mallocarray likely should use WOULD_OVERFLOW instead to do proper error handling, assuming it is fixed to match the actual malloc interface. This is especially true in the NO_WAIT cases. Warner