From owner-svn-src-all@freebsd.org Fri May 25 17:56:31 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 6A646EEF809; Fri, 25 May 2018 17:56:31 +0000 (UTC) (envelope-from araujobsdport@gmail.com) Received: from mail-wr0-x22d.google.com (mail-wr0-x22d.google.com [IPv6:2a00:1450:400c:c0c::22d]) (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 D5CE86C475; Fri, 25 May 2018 17:56:30 +0000 (UTC) (envelope-from araujobsdport@gmail.com) Received: by mail-wr0-x22d.google.com with SMTP id l41-v6so10607607wre.7; Fri, 25 May 2018 10:56:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:reply-to:in-reply-to:references:from:date:message-id :subject:to:cc; bh=bsw/xrc2few9ScgwIsvhiWF4MDEgASwE/hgKKIVkn9g=; b=Sz4H0NcG/gmEmsFO6kCrywW/erRp1r0/gv9NAxL1Lw0TBSJSyOiP6AJCJwj+li4cm8 VjG9oL/QWFbOWwcQPYVo9PA/OIJU05JAw4XOivlLz1LTz7Hy5Latwng9Lgj8N5fppt7N CGJogcquWw9ZBY8PC3dNL21+NeVPLCLUjTzVIyHJR3snQZVDi+EQwuBtyaWvv4RThsa0 q/AWVHYTf7Xfq0J1knwvhyNtVBQJCl55f6YOsPN6t0tGtQc8+Ozk1uKt29WU7Jp52wh2 ZMt/Wz1ywZAZYlyp32I1lPIlJ2DbN1XuMy3EDLRvG/Q/+11tACouo0KUN3NZ41grepOZ 1MCg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:reply-to:in-reply-to:references :from:date:message-id:subject:to:cc; bh=bsw/xrc2few9ScgwIsvhiWF4MDEgASwE/hgKKIVkn9g=; b=EK2erR26PW9tq0ftOrGfBjvjjVgo8CqGEfP1EwG8MZq37cJjrIkFOVR57AAxd+nZhM hXPS/OBX7JVZFLVtOY6Z1e+BFqpzZJ6b0axBsEU6DVpRq2vjjwWC/THTKT5SNR7RIMZl zl8OsBwGISqq8nxCU1PqJELr/4wOuT3f92hsdAGkQWw0+zKC7hou+055uOhFCP1rXLGZ qXdtdjvDzzYMY1rEhzbODMlxAhHWmf5/DGQAQOiV+bhaUgV5nB5iBB1zkyqWK93AuPBr N0tUl+8w4BFJu2Ba1Rgrl+cbvcWmQiYJV/X+PwflY94BbyqjmV5wZ5zeECMu6o0V4XaD qd1w== X-Gm-Message-State: ALKqPwdAhA9LvIY2N85xWkSewZdtPtCnSewz0EVQ9ZAzeKwP4svB3eJN jdgNQt+ucyvWji1YkaVZ4O9l+AuF6eih5PyKBBAaqQ== X-Google-Smtp-Source: ADUXVKIQSr3N9OSo/c+7zHGWyEWGYeBUoLAKXGokn/vlSqQVdhvlghJ4rGLWyYiPYhfjCNUyt4LP+wcugA06UOkxF68= X-Received: by 2002:a19:9a10:: with SMTP id c16-v6mr2094205lfe.60.1527270988726; Fri, 25 May 2018 10:56:28 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a19:1fc9:0:0:0:0:0 with HTTP; Fri, 25 May 2018 10:56:28 -0700 (PDT) Reply-To: araujo@freebsd.org In-Reply-To: <20180525174424.GD99063@spindle.one-eyed-alien.net> References: <201805250207.w4P275Pf060725@repo.freebsd.org> <20180525151134.GB99063@spindle.one-eyed-alien.net> <20180525174424.GD99063@spindle.one-eyed-alien.net> From: Marcelo Araujo Date: Sat, 26 May 2018 01:56:28 +0800 Message-ID: Subject: Re: svn commit: r334199 - head/usr.sbin/bhyve To: Brooks Davis Cc: Eitan Adler , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.26 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.26 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: Fri, 25 May 2018 17:56:31 -0000 2018-05-26 1:44 GMT+08:00 Brooks Davis : > On Sat, May 26, 2018 at 01:21:33AM +0800, Marcelo Araujo wrote: > > On Sat, May 26, 2018, 1:11 AM Eitan Adler wrote: > > > > > On 25 May 2018 at 08:23, Marcelo Araujo > wrote: > > > > > > > > > > > > On Fri, May 25, 2018, 11:11 PM Brooks Davis > wrote: > > > >> > > > >> On Fri, May 25, 2018 at 02:07:05AM +0000, Marcelo Araujo wrote: > > > >> > Author: araujo > > > >> > Date: Fri May 25 02:07:05 2018 > > > >> > New Revision: 334199 > > > >> > URL: https://svnweb.freebsd.org/changeset/base/334199 > > > >> > > > > >> > Log: > > > >> > Fix a memory leak on topology_parse(). > > > >> > > > > >> > strdup(3) allocates memory for a copy of the string, does the > copy > > > and > > > >> > returns a pointer to it. If there is no sufficient memory NULL > is > > > >> > returned > > > >> > and the global errno is set to ENOMEM. > > > >> > We do a sanity check to see if it was possible to allocate > enough > > > >> > memory. > > > >> > > > > >> > Also as we allocate memory, we need to free this memory used. > Or it > > > >> > will > > > >> > going out of scope leaks the storage it points to. > > > >> > > > > >> > Reviewed by: rgrimes > > > >> > MFC after: 3 weeks. > > > >> > X-MFC: r332298 > > > >> > Sponsored by: iXsystems Inc. > > > >> > Differential Revision: https://reviews.freebsd.org/D15550 > > > >> > > > > >> > Modified: > > > >> > head/usr.sbin/bhyve/bhyverun.c > > > >> > > > > >> > Modified: head/usr.sbin/bhyve/bhyverun.c > > > >> > > > > >> > > > > ============================================================ > ================== > > > >> > --- head/usr.sbin/bhyve/bhyverun.c Fri May 25 01:38:59 2018 > > > >> > (r334198) > > > >> > +++ head/usr.sbin/bhyve/bhyverun.c Fri May 25 02:07:05 2018 > > > >> > (r334199) > > > >> > @@ -193,6 +193,7 @@ topology_parse(const char *opt) > > > >> > c = 1, n = 1, s = 1, t = 1; > > > >> > ns = false, scts = false; > > > >> > str = strdup(opt); > > > >> > + assert(str != NULL); > > > >> > > > >> Using assert seems like an odd choice when you've already added a > > > >> failure path and the strsep will crash immediately if assert is > elided. > > > > > > > > > > > > Just to make a better point, I had the same discussion about > assert(3) in > > > > another review, we don't do NDEBUG even for RELEASE. > > > > > > IMHO we only use assert for asserting things ought to never be false > > > except in buggy code. Using assert for handling is poor practice. > > > > > > > Again, in this case we are using it all over the place and we must > replace > > it. Also we should document it in somewhere perhaps in the assert(3) > > otherwise myself and others will keep using it. If you use find, not only > > myself is using it to check strdup! So what is the suggestion to handle > > assert(3)? Deprecated it? > > Code that uses assert() in place of error handling is wrong and should > be fixed. assert(condition) means that condition must never happen > and if it does a bug has occurred (or the programmers assumptions are > wrong). In this case failure would not be due to a bug, but do to > resource exhaustion which is expected to be handled. > I agree with you! We have plenty of place that use strdup(3) without check the errno ENOMEN return; so do you think would be better bypass a errno ENOMEN without check it and have a crash, or better abort(3) using assert(3) in case we have no memory available to allocated the memory for a copy of a string? Personally I don't mind make couple extra lines of code to call abort(3) or exit(3), but till there, if we don't make RELEASE using NDEBUG, what you guys are saying to me is more personal preference than anything else. > > -- Brooks > -- -- Marcelo Araujo (__)araujo@FreeBSD.org \\\'',)http://www.FreeBSD.org \/ \ ^ Power To Server. .\. /_)