NetBSD Problem Report #50571

From www@NetBSD.org  Fri Dec 18 20:51:36 2015
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.NetBSD.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id ACCE17ACD6
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 18 Dec 2015 20:51:36 +0000 (UTC)
Message-Id: <20151218205135.9EAFE7ACDC@mollari.NetBSD.org>
Date: Fri, 18 Dec 2015 20:51:35 +0000 (UTC)
From: dcb314@hotmail.com
Reply-To: dcb314@hotmail.com
To: gnats-bugs@NetBSD.org
Subject: src/sys/fs/udf/udf_subr.c:6465: obvious performance tidyup
X-Send-Pr-Version: www-1.0

>Number:         50571
>Category:       kern
>Synopsis:       src/sys/fs/udf/udf_subr.c:6465: obvious performance tidyup
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Dec 18 20:55:00 +0000 2015
>Closed-Date:    Sat Dec 19 03:21:48 +0000 2015
>Last-Modified:  Sat Dec 19 03:30:00 +0000 2015
>Originator:     David Binderman
>Release:        cvs dated 20151218
>Organization:
>Environment:
>Description:
[src/sys/fs/udf/udf_subr.c:6465] -> [src/sys/fs/udf/udf_subr.c:6466]: (performance) Buffer 'blob' is being written before its old content has been used.

Source code is

    assert(inflen < sector_size);

    /* copy out info */
    memset(blob, 0, sector_size);
    memcpy(blob, pos, inflen);

Maybe better code

    assert(inflen < sector_size);

    /* copy out info */
    memcpy(blob, pos, inflen);
    memset(&blob[inflen] 0, sector_size - inflen);

>How-To-Repeat:

>Fix:

>Release-Note:

>Audit-Trail:
From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/50571: src/sys/fs/udf/udf_subr.c:6465: obvious performance tidyup
Date: Sat, 19 Dec 2015 04:23:36 +0700

     Date:        Fri, 18 Dec 2015 20:55:00 +0000 (UTC)
     From:        dcb314@hotmail.com
     Message-ID:  <20151218205500.69D4A7AC07@mollari.NetBSD.org>

   | Maybe better code
   | 
   |     assert(inflen < sector_size);
   | 
   |     /* copy out info */
   |     memcpy(blob, pos, inflen);
   |     memset(&blob[inflen] 0, sector_size - inflen);

 That wouldn't be enough, you'd also need

 	memset(blob, 0, pos);

 But unless inflen is likely to be of a magnitde comparable to sector_size,
 and unless both a relatively large (or likely to be) then I don't think
 any change is worth the bother.

 Clearing an entire struct/array before then assigning other values to some
 elements is standard practice - if this is offending any automated checking
 code, that ought to be fixed.   If it is just your sensibilities, get over
 it.

 kre


From: Joerg Sonnenberger <joerg@britannica.bec.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/50571: src/sys/fs/udf/udf_subr.c:6465: obvious performance
 tidyup
Date: Fri, 18 Dec 2015 23:22:54 +0100

 On Fri, Dec 18, 2015 at 09:25:01PM +0000, Robert Elz wrote:
 >  Clearing an entire struct/array before then assigning other values to some
 >  elements is standard practice - if this is offending any automated checking
 >  code, that ought to be fixed.   If it is just your sensibilities, get over
 >  it.

 Compilers are also starting to recognize this pattern and optimize
 redundant writes accordingly.

 Joerg

From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, 
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, dcb314@hotmail.com
Cc: 
Subject: Re: kern/50571: src/sys/fs/udf/udf_subr.c:6465: obvious performance tidyup
Date: Fri, 18 Dec 2015 20:12:59 -0500

 On Dec 18, 10:25pm, joerg@britannica.bec.de (Joerg Sonnenberger) wrote:
 -- Subject: Re: kern/50571: src/sys/fs/udf/udf_subr.c:6465: obvious performan

 |  Compilers are also starting to recognize this pattern and optimize
 |  redundant writes accordingly.

 helping the compiler to produce more efficient code has a better chance of
 success today.

 christos

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/50571 CVS commit: src/sys/fs/udf
Date: Fri, 18 Dec 2015 20:18:00 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Sat Dec 19 01:18:00 UTC 2015

 Modified Files:
 	src/sys/fs/udf: udf_subr.c

 Log Message:
 PR/50571: David Binderman: src/sys/fs/udf/udf_subr.c:6465: obvious
 performance tidyup


 To generate a diff of this commit:
 cvs rdiff -u -r1.132 -r1.133 src/sys/fs/udf/udf_subr.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Robert Elz <kre@munnari.OZ.AU>
To: christos@zoulas.com (Christos Zoulas)
Cc: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
        netbsd-bugs@netbsd.org, dcb314@hotmail.com
Subject: Re: kern/50571: src/sys/fs/udf/udf_subr.c:6465: obvious performance tidyup
Date: Sat, 19 Dec 2015 09:53:37 +0700

     Date:        Fri, 18 Dec 2015 20:12:59 -0500
     From:        christos@zoulas.com (Christos Zoulas)
     Message-ID:  <20151219011259.AD7F117FDAB@rebar.astron.com>

   | helping the compiler to produce more efficient code has a better chance of
   | success today.

 I wouldn't guarantee that it is a performance improvement, it depends
 upon the size and alignment of pos & inflen, if inflen is relatively
 small, and pos isn't aligned (and nor is pos+inflen) then the extra
 costs of the memset machinations required are likely to outweigh the
 cost of a few more 0 writes.

 Aside from the question of whether it should have been done, you also
 just inserted the code supplied in the initial PR from dcb314@hotmail.com
 which leaves blob[0 ..  pos-1] uninitialised.

 It needs an extra memset to correct that.

 kre

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/50571: src/sys/fs/udf/udf_subr.c:6465: obvious performance
 tidyup
Date: Sat, 19 Dec 2015 03:10:13 +0000

 On Sat, Dec 19, 2015 at 02:55:01AM +0000, Robert Elz wrote:
  >  Aside from the question of whether it should have been done, you also
  >  just inserted the code supplied in the initial PR from dcb314@hotmail.com
  >  which leaves blob[0 ..  pos-1] uninitialised.

 It's not memcpying to blob + pos, it's memcpying to blob. "pos" is the
 source pointer.

 boo on confusing names.

 (that the code also didn't compile is perhaps another issue but it's
 been fixed already)

 -- 
 David A. Holland
 dholland@netbsd.org

From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/50571 CVS commit: src/sys/fs/udf
Date: Sat, 19 Dec 2015 03:16:09 +0000

 Module Name:	src
 Committed By:	dholland
 Date:		Sat Dec 19 03:16:09 UTC 2015

 Modified Files:
 	src/sys/fs/udf: udf_subr.c

 Log Message:
 Improve misleading variable name. Related to PR 50571.

 XXX: also there should be real bounds-check logic in here.
 XXX: if the on-disk data structure contains rubbish this code will
 XXX: leak or trample arbitrary kernel memory.


 To generate a diff of this commit:
 cvs rdiff -u -r1.134 -r1.135 src/sys/fs/udf/udf_subr.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sat, 19 Dec 2015 03:21:48 +0000
State-Changed-Why:
Fixed, thanks.


From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/50571: src/sys/fs/udf/udf_subr.c:6465: obvious performance tidyup
Date: Sat, 19 Dec 2015 10:27:22 +0700

     Date:        Sat, 19 Dec 2015 03:15:00 +0000 (UTC)
     From:        David Holland <dholland-bugs@netbsd.org>
     Message-ID:  <20151219031500.E860C7ACE4@mollari.NetBSD.org>

   |  It's not memcpying to blob + pos, it's memcpying to blob. "pos" is the
   |  source pointer.

 Oh, right, I misread the original patch in the PR, apologies to all.

 kre

>Unformatted:

Home	PR Database Search