Re: pdksh

[David Sainty <david.sainty%gmail.com@localhost>]

Hi Richard,

> http://www.netbsd.org/~richard/pdksh.patch
>
> The above patch is the pkgsrc specific WIP of a tiny bit of discussion in tech-userlevel@
>
> I'd like to sollicit any comments/observations prior to committing.
> I've been using this for a bit now.

--- files/table.h
+++ files/table.h
@@ -6,7 +6,7 @@
 
 struct table {
 	Area   *areap;		/* area to allocate entries */
-	short	size, nfree;	/* hash size (always 2^^n), free entries */
+	int size, nfree;	/* hash size (always 2^^n), free entries */
 	struct	tbl **tbls;	/* hashed table items */
 };


Indeed!  This bit of your patch is absolutely identical to mine.  So yes, with this patch in place you won't see the crash either.  Good work :)  Shame I hadn't noticed you'd done it first...

--- files/table.c
+++ files/table.c
@@ -8,6 +8,10 @@
 
 #define	INIT_TBLS	8	/* initial table size (power of 2) */
 
+#ifndef	min
+#define	min(a, b)	((a) > (b) ? (b) : (a))
+#endif
+
 static void     texpand     ARGS((struct table *tp, int nsize));
 static int      tnamecmp    ARGS((void *p1, void *p2));
 
@@ -116,7 +120,7 @@
 	}
 
 	if (tp->nfree <= 0) {	/* too full */
-		texpand(tp, 2*tp->size);
+		texpand(tp, 2 * min(INT_MAX/2, tp->size));
 		goto Search;
 	}

This part I have some problems with, I don't think it should go in.

1. The size needs to be 2^(int)n - texpand() assumes that.  INT_MAX/2*2 is not 2^(int)n, so it's buggy if the min() operation ever caps the value to that.
2. Ignoring Bug #1, it would still lead to the table being (uselessly) expanded to the same size forever, for each and every new variable once the limit is hit, because the table would never actually expand past INT_MAX.  That wouldn't perform well(!).
3. Bugs #1 and #2 aside, I'm not sure how much sleep it's worth losing over the possibility of INT_MAX shell variables, that's a much much harder limit to reach.  The bug is probably about 20 years old and the INT16_MAX limit is only just getting attention :)

If seriously trying to avoid an INT_MAX limit being hit, I'd go with using size_t instead of int [requires a much bigger code change].  That way you know for sure that the type's size is not going to be the first limit hit - and you can avoid implementing code to deal with the (possibly never going to happen) case of INT_MAX+1 variables.

The rest of the patch is involved with code I haven't paid much 
attention to.  I'm not sure what to say about those, it's quite a big 
patch!  Some looks ok, some looks a little odd.  For example all the 
(null) -> ("%s", null) changes seem a bit questionable to me, because if 
(null) wasn't valid why was it coded that way to start with?

Code that's worked for decades probably deserves test cases though :)  
Have any been done?

Cheers,

Dave