Re: [patch] Error on setenv(..., NULL, ...)

[Paul Eggert <eggert at cs dot ucla dot edu>]

On 03/12/2015 02:53 PM, Roland McGrath wrote:
> The only reason I can see not
> to do that is to force the fault to be before the lock is taken.  If that
> is the explicit intent of the code, then its comments should say so.

I had assumed that lengths are computed before locking to minimize the 
size of the critical section, making it less of a bottleneck. Attached 
is a revised (untested) patch with comments saying that.
>From 1441ea7a5cfb78e36c8e6f170a30de52fd01920e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 12 Mar 2015 15:57:07 -0700
Subject: [PATCH] * stdlib/setenv.c (__add_to_environ):

Dump core quickly if setenv (..., NULL, ...) is called.
---
 ChangeLog       |  5 +++++
 stdlib/setenv.c | 10 +++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index d6526e9..56b3d73 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2015-03-12  Paul Eggert  <eggert@cs.ucla.edu>
+
+	* stdlib/setenv.c (__add_to_environ):
+	Dump core quickly if setenv (..., NULL, ...) is called.
+
 2015-03-12  Joseph Myers  <joseph@codesourcery.com>
 
 	* soft-fp/soft-fp.h (_FP_STATIC_ASSERT): New macro.
diff --git a/stdlib/setenv.c b/stdlib/setenv.c
index b60c4f0..0534236 100644
--- a/stdlib/setenv.c
+++ b/stdlib/setenv.c
@@ -114,8 +114,16 @@ __add_to_environ (name, value, combined, replace)
 {
   char **ep;
   size_t size;
+
+  /* Compute lengths before locking, so that the critical section is
+     less of a performance bottleneck.  VALLEN is needed only if
+     COMBINED is non-null.  Also, testing COMBINED instead of VALUE
+     causes setenv (..., NULL, ...) to dump core now instead of
+     corrupting memory later.  */
   const size_t namelen = strlen (name);
-  const size_t vallen = value != NULL ? strlen (value) + 1 : 0;
+  size_t vallen;
+  if (combined != NULL)
+    vallen = strlen (value) + 1;
 
   LOCK;
 
-- 
2.1.0