From owner-svn-src-all@freebsd.org Wed Jan 24 18:05:27 2018 Return-Path: Delivered-To: svn-src-all@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 B7229EB9261 for ; Wed, 24 Jan 2018 18:05:26 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-wm0-x234.google.com (mail-wm0-x234.google.com [IPv6:2a00:1450:400c:c09::234]) (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 281696CFC3 for ; Wed, 24 Jan 2018 18:05:26 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-wm0-x234.google.com with SMTP id v123so10295698wmd.5 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=r9eniv06P/ulxTXJB1H4duPsMfs+qeVlP4f2nRzcJiLg3rBPEDGM2u4ex2hcoh7rPc EI8JqE44V+8+UFU307YxbN3H8jGZTrCuuSP6pafB6U6JL1PPnosv32ZGq0pl9xprodbP PHFxxvhW1q3zjq8E6MW/1uOqCHZYvLFKwvQnDhLjP8FW/YfMQD/ZVNmMFa4/8WBy2yLE phF13IHEe3Jhue0XbRs5PTc5RlXUQuZ+unSXj8jSHa8y4WDaGZGJM9hfJHAlOcEJPmmO QS/F+OzWXQL9cw04TbgVK4aLqvWPP1tt+1JruOIzMnxy/+iTTcXOsgJohMVXnWh1Al+0 ltaA== X-Gm-Message-State: AKwxytf0oihxkTUg3ehBLmD9xWEHu3jpJlHzkOGwg+l3iwGq7nCBo6MR Rlyra4e2BOkg+8A/fwAJfoZKgJZPImmsVQs4tE5Msuqf 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-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" 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