NetBSD Problem Report #50934

From www@NetBSD.org  Thu Mar 10 18:27:50 2016
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 D04E27ABF5
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 10 Mar 2016 18:27:50 +0000 (UTC)
Message-Id: <20160310182749.6E73C7ACA1@mollari.NetBSD.org>
Date: Thu, 10 Mar 2016 18:27:49 +0000 (UTC)
From: marcotte@panix.com
Reply-To: marcotte@panix.com
To: gnats-bugs@NetBSD.org
Subject: pkill/pgrep segfaults in some cases
X-Send-Pr-Version: www-1.0

>Number:         50934
>Category:       bin
>Synopsis:       pkill/pgrep segfaults in some cases
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kre
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Mar 10 18:30:00 +0000 2016
>Closed-Date:    Thu Aug 17 18:36:34 +0000 2017
>Last-Modified:  Thu Aug 17 18:36:34 +0000 2017
>Originator:     Brian Marcotte
>Release:        7.0
>Organization:
Public Access Networks, Corp.
>Environment:
NetBSD panix5.panix.com 7.0 NetBSD 7.0 (PANIX-XEN-USER) #1: Tue Jan 19 00:58:25 EST 2016  root@juggler.panix.com:/misc/obj/misc/devel/netbsd/7.0/src/sys/arch/i386/compile/PANIX-XEN-USER 
>Description:
pkill/pgrep can sigfault when looking at the list of processes.

Somehow it's possible that pargv[0] can be NULL and it segfaults on this line:

    strlcpy(buf, pargv[0], sizeof(buf));

That appeared in revision 1.28 of pkill.c


>How-To-Repeat:
I don't know how to duplicate the process which is causing it's argv[0] to be NULL, but I can test changes on the machine that currently has the issue.

>Fix:
Perhaps fall back to using p_comm when pargv[0] is NULL:


--- /devel/netbsd/7.0/src/usr.bin/pkill/pkill.c 2015-01-27 08:39:31.000000000 -0500
+++ pkill.c     2016-03-10 13:19:17.000000000 -0500
@@ -296,8 +296,12 @@
                                            pargv[0]);
                                        pargv++;
                                }
-                       } else
-                               strlcpy(buf, pargv[0], sizeof(buf));
+                       } else {
+                               if (pargv[0] == NULL)
+                                       strlcpy(buf, kp->p_comm, sizeof(buf));
+                               else
+                                       strlcpy(buf, pargv[0], sizeof(buf));
+                       }

                        rv = regexec(&reg, buf, 1, &regmatch, 0);
                        if (rv == 0) {

>Release-Note:

>Audit-Trail:
From: Brian Marcotte <marcotte@panix.com>
To: gnats-admin@netbsd.org
Cc: gnats-bugs@NetBSD.org
Subject: Re: bin/50934: pkill/pgrep segfaults in some cases
Date: Mon, 20 Feb 2017 21:34:33 -0500

 > >Synopsis:       pkill/pgrep segfaults in some cases
 > >Arrival-Date:   Thu Mar 10 18:30:00 +0000 2016

 Any update on this? I just encountered this again.

 Thanks.

 --
 - Brian

From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/50934: pkill/pgrep segfaults in some cases
Date: Tue, 21 Feb 2017 13:50:25 +0700

     Date:        Tue, 21 Feb 2017 02:35:01 +0000 (UTC)
     From:        Brian Marcotte <marcotte@panix.com>
     Message-ID:  <20170221023501.1B7987A291@mollari.NetBSD.org>

   |  Any update on this? I just encountered this again.

 Taking a look now.   The patch you supplied looks reasonable (though
 I will install it, assuming the build I am doing now to test it works,
 in a slightly different form).

 argv[0] can be NULL as processes are permitted to do anything they like
 to their arg vector.   If you want a guaranteed argv[0] == NULL test
 case, try building and running ...


 	main(int argc, char **argv)
 	{
 		argv[0] = 0;

 		pause();
 		exit(0);
 	}

 (kill it with a signal, like SIGINT, when you're done .. and to compile
 cleanly it probably needs <unistd.h> or something included to get the
 pause() and exit() prototypes - if you just cc it, without -W args (and
 it won't really be helped by optimisation...) it should be fine as is.

 All other uses of the arg vector in the code, except that one, look to
 be protected - though only against NULL, a program that does ...

 		argv[0] = 0xdeadbeef;

 might still cause problems (but if so, that will be a problem inside
 the kvm library).

 kre

Responsible-Changed-From-To: bin-bug-people->kre
Responsible-Changed-By: kre@NetBSD.org
Responsible-Changed-When: Tue, 21 Feb 2017 09:33:22 +0000
Responsible-Changed-Why:
I am looking into this PR


State-Changed-From-To: open->analyzed
State-Changed-By: kre@NetBSD.org
State-Changed-When: Tue, 21 Feb 2017 09:34:08 +0000
State-Changed-Why:
problem understood as reported, fix imminent


From: Robert Elz <kre@munnari.OZ.AU>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/50934: pkill/pgrep segfaults in some cases
Date: Tue, 21 Feb 2017 16:32:09 +0700

 I will soom be committing an ATF test for this bug - by the time it
 is committed, it will have been cofirmed that the test works correctly
 when the bug is fixed (I have already confirmed the fix for the bug).

 The test is based upon the code fragment in the previous message
 (it is stdlib.h it needs, not unistd.h - perhaps one day I will learn
 which appiles when .... but probably not - and yes, I did #include it)
 slightly enhanced to make it a better test.

 I will commit the fix after babylon5 has done a build that includes the
 test (which should then fail when it is run).  A subsequent test run
 after the fix is included should indicate the test "no longer failing".

 The fix will need pullups to netbsd-7 and netbsd-7-0 (I will request
 those after the fix has been demnonstrated to work.)

 kre

From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/50934 CVS commit: src
Date: Tue, 21 Feb 2017 10:40:30 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Tue Feb 21 10:40:30 UTC 2017

 Modified Files:
 	src/distrib/sets/lists/tests: mi
 	src/etc/mtree: NetBSD.dist.tests
 	src/tests/usr.bin: Makefile
 Added Files:
 	src/tests/usr.bin/pkill: Makefile t_pgrep.sh

 Log Message:
 PR bin/50934

 Add a test program for the bug described in this PR.
 This is the first pkill/pgrep/prenice test (more would be good!)

 This test has been confirmed to work once the bug described in the PR
 has been fixed, so the test is not marked "expected to fail" even
 though initially that is what should happen.

 Note: the test cana also fail if the system running the tests happens
 to be running processes with names that match the patterns searched for
 by the test, other than the test program itself.  This is expected to be
 unlikely.


 To generate a diff of this commit:
 cvs rdiff -u -r1.722 -r1.723 src/distrib/sets/lists/tests/mi
 cvs rdiff -u -r1.141 -r1.142 src/etc/mtree/NetBSD.dist.tests
 cvs rdiff -u -r1.23 -r1.24 src/tests/usr.bin/Makefile
 cvs rdiff -u -r0 -r1.1 src/tests/usr.bin/pkill/Makefile \
     src/tests/usr.bin/pkill/t_pgrep.sh

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

From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/50934 CVS commit: src/usr.bin/pkill
Date: Tue, 21 Feb 2017 13:09:56 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Tue Feb 21 13:09:56 UTC 2017

 Modified Files:
 	src/usr.bin/pkill: pkill.c

 Log Message:
 PR bin/50934 -- avoid core dump if a process being examined has set
 its argv[0] to NULL.

 XXX Needs pullups netbsd-7-0 and netbsd-7 (bug was introduced after
     netbsd-6 and never pulled up, so no netbsd-6 pullups required.)


 To generate a diff of this commit:
 cvs rdiff -u -r1.30 -r1.31 src/usr.bin/pkill/pkill.c

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

State-Changed-From-To: analyzed->feedback
State-Changed-By: kre@NetBSD.org
State-Changed-When: Tue, 21 Feb 2017 22:34:17 +0000
State-Changed-Why:

Since I totally botched the procedure I planned -- to use bablylon5 to
verify the bug & its fix (botched by breaking the test in an attempt to
avoid a meaningless warning and not taking the time to properly verify
my change), I will instead ask if you can check that the fix committed
for this solves the problem for you.

Since it is basically your fix from the PR, I tend to assume it does,
and I did verify the bug existed on my system, and that it was fixed
(with a properly coded version of the test, and by running the test
program standalone before it was added to ATF)  I believe that the
fix is fine, but ...

kre


From: Brian Marcotte <marcotte@panix.com>
To: kre@NetBSD.org
Cc: netbsd-bugs@netbsd.org, gnats-admin@netbsd.org, gnats-bugs@NetBSD.org
Subject: Re: bin/50934 (pkill/pgrep segfaults in some cases)
Date: Wed, 22 Feb 2017 02:15:57 -0500

 > my change), I will instead ask if you can check that the fix committed
 > for this solves the problem for you.

 Yes, it works fine.

 Thanks!

 --
 - Brian

State-Changed-From-To: feedback->pending-pullups
State-Changed-By: kre@NetBSD.org
State-Changed-When: Wed, 22 Feb 2017 11:13:55 +0000
State-Changed-Why:

Thanks for the confirmation.

Now waiting on pullups to netbsd-7 and netbsd-7-0  (request number 1365)


From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/50934 CVS commit: [netbsd-7] src/usr.bin/pkill
Date: Sat, 18 Mar 2017 05:13:22 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Sat Mar 18 05:13:22 UTC 2017

 Modified Files:
 	src/usr.bin/pkill [netbsd-7]: pkill.c

 Log Message:
 Pull up following revision(s) (requested by kre in ticket #1365):
 	usr.bin/pkill/pkill.c: revision 1.31
 PR bin/50934 -- avoid core dump if a process being examined has set
 its argv[0] to NULL.


 To generate a diff of this commit:
 cvs rdiff -u -r1.29.8.1 -r1.29.8.2 src/usr.bin/pkill/pkill.c

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

State-Changed-From-To: pending-pullups->closed
State-Changed-By: jdolecek@NetBSD.org
State-Changed-When: Thu, 17 Aug 2017 18:36:34 +0000
State-Changed-Why:
Pullup to netbsd-7 done in March 2016. 7.0 pullup not done, but the change
is in 7.1.


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2014 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.