From owner-svn-src-all@freebsd.org Sun Jan 27 23:38:57 2019 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 A2FA414C1323 for ; Sun, 27 Jan 2019 23:38:57 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qk1-x72c.google.com (mail-qk1-x72c.google.com [IPv6:2607:f8b0:4864:20::72c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 43DE38B9A1 for ; Sun, 27 Jan 2019 23:38:57 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qk1-x72c.google.com with SMTP id 68so8445808qke.9 for ; Sun, 27 Jan 2019 15:38:57 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=JShXvvmrJuLImVG9P9sqt7uNcqbxfXL8KTCORV8im5k=; b=fs6ZxzlZCLC1atJXTFB9sq1gejo0ceN7K+u4kaz3M2T3TAeaDqZa0aXheoOVuJwAUD hRNtwXStEOkKQQb5DO6S1RWGP3087vMhdB5KS2qg0bS2rVg7V7JdFBF8+5TqAABQ3U4b FUGec6RCfjTB5ujFkPRIIxPKDd91XUmEhxEDNGYoT4hQjZw9dlD6ReNaN2iO6TuulBmQ BonvicBDl2M+DnyfbkAA5W/tvjc8Q2Xav3qrV9/rA4bWeaQCiZ2VQYK4ZeHZfKaiDmpA X6600udAmACB+X3noq0QK8MBJoD8NUhvVUV9lVXAWdPYTlQ64yG6ht5GX/zh3T7cLPZt nNAQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=JShXvvmrJuLImVG9P9sqt7uNcqbxfXL8KTCORV8im5k=; b=I5W5TZsJ+/k2R3tKzPWCIJjHr2VzPDL+H8VOzbgkqVJseCx/TLKH5BWs57n7IrLr5N k45Qc6YqEUyQ5Jp0wkcvHl08N6+AudhflcrQn+WjuOuCVp/eWv+mfCmti6TgPqy0Dfqd tFVxOVaeoYqhkyYleWjb2+eOqkvmpH9/pFOVSsuDN7OMb85LTOCkCjncZN0dMxe9oxaj 4+nT061nDggln7/G99IoHlqHBsNuWdHBoJ7ZwxrUJZr3UwmCScA9EeuWjVnb6hqXZuGl xdlFE9Ae049VhfE3N1OnrFhHS1cfnyVneFdrVt/EL+WXbh/l6zbLbMO5AkkwEXsPOo4C RfQA== X-Gm-Message-State: AJcUukcc7EIkh99HL0aazXnBEvfFNG00zB8ayWtvRdoLeFYmca4oggCg 5IPVCp9MlbpiUhixGViEbXhJScjzwGpfYc6lq4ne8w== X-Google-Smtp-Source: ALg8bN5x4dKJuNuuMb8+8erZGgCsN55Qs6LYm9xtQkiFhWry03WFa+ayloLezQMTHz5ZGzDl13dwhxaYzt8EK1WM6X0= X-Received: by 2002:a37:6c05:: with SMTP id h5mr17735428qkc.175.1548632336406; Sun, 27 Jan 2019 15:38:56 -0800 (PST) MIME-Version: 1.0 References: <201901262136.x0QLaAJv095518@pdx.rh.CN85.dnsmgr.net> <010001688c2cfc3e-e319d851-8b9e-4468-8bd1-f93331f35116-000000@email.amazonses.com> <20190127154047.V900@besplex.bde.org> In-Reply-To: From: Warner Losh Date: Sun, 27 Jan 2019 16:38:45 -0700 Message-ID: Subject: Re: svn commit: r343480 - head/lib/libfigpar To: Devin Teske Cc: Bruce Evans , Stefan Esser , Colin Percival , "Rodney W. Grimes" , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org X-Rspamd-Queue-Id: 43DE38B9A1 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.99 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.99)[-0.987,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 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: Sun, 27 Jan 2019 23:38:57 -0000 On Sun, Jan 27, 2019 at 7:09 AM Devin Teske wrote: > > > > On Jan 26, 2019, at 9:31 PM, Bruce Evans wrote: > > > > On Sat, 26 Jan 2019, Stefan Esser wrote: > > > >> Am 26.01.19 um 22:59 schrieb Colin Percival: > >>> ... > >>> The length of the string was already being recalculated, by strcpy. > >>> > >>> It seems to me that this could be written as > >>> > >>> temp = malloc(slen + 1); > >>> if (temp == NULL) /* could not allocate memory */ > >>> return (-1); > >>> memcpy(temp, source, slen + 1); > >>> > >>> which avoids both recalculating the string length and using strcpy? > >> > >> In principle you are right, there is a small run-time cost by not using > >> the known length for the allocation. > >> > >> The Clang Scan checks are leading to lots (thousands) of false positives > >> and a I have looked at quite a number and abstained from silencing them > >> in the hope that the scan will be improved. > >> > >> It should learn that at least the trivial case of an allocation of the > >> value of strlen()+1 followed by a strcpy (and no further strcat or the > >> like) is safe. > > > > It would be a large bug in coverity for it complain about normal use of > > strcpy(). > > > > However, the the use was not normal. It has very broken error checking > > for malloc(). This gave undefined behaviour for C99 and usually failure > > of the function and a memory leak for POSIX, > > > > The broken error checking was to check errno instead of the return > > value of malloc(), without even setting errno to 0 before calling > > malloc(). This is especially broken in a library. It is normal for > > errno to contain some garbage value from a previous failure, e.g., > > ENOTTY from isatty(). So the expected behaviour of this library > > function is to usually fail and leak the successfully malloc()ed memory > > when the broken code is reached. Coverity should find this bug. > > Perhaps it did. > > > > If errno were set before the call to malloc(), then the code would just > > be unportable. Setting errno when malloc() fails is a POSIX extension > > of C99. Coverity should be aware of the standard being used, and should > > find this bug for C99 but not for POSIX. > > > > The fix and the above patch don't fix styles bug related to the broken > > check. First there is the lexical style bug of placing the comment > > on the check instead of on the result of the check. The main bug is > > that it should go without saying that malloc failures are checked for > > by checking whether malloc() returned NULL. But in the old version, > > the check was encrypted as the broken check of errno. The comment > > decrypts this a little. > > > > I am genuinely flattered that so much thought is being placed around code > that I wrote circa 1999. > > My code there might even pre-date the C99 standard if memory serves. > > When I wrote that code, I had tested it on extensively on over 20 > different UNIX platforms. > > Compatibility was a nightmare back then. I'm so happy we have all these > wonderful shiny standards today. > Yea, Unix compatibility was great through '77 or so (since even though there was some minor diversity inside of Bell Labs, it wasn't so large and the extant code base was small enough to cope). The jump form V6 to V7 broke a lot of interfaces, so people started doing portability hacks. It got worse as BSD released its evolved versions, and USG started to go in a different direction with System III. Then the Unix Wars happened during the 80's. By 1999 these had mostly settled out and most of the portability mess as but a mere echo of what it had grown to in the early 80's (the curious can go look at the TTY interface diversity that Kermit pandered to until the late 90's when it grew too large to run on most of the crazy systems it had compat code for and started trimming the ancient ones... I think it supported 6 or 7 different variants of Unix that ran on the PDP-11 alone... though to be fair, the tty interface was/is the hardest to write portably, even today). Today, the fast moving target is all the new Linux system calls and weird library routines, but much of that makes it into standards bodies and FreeBSD keeps up enough to knock most of the rough edges off... Warner