tech-userlevel archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
pdksh
After first soliciting Amitai's and Tobias' opinion, it was felt best to start here.
First the problem statement:
I came across some issues with pkgsrc using pdksh as primary TOOLS_PLATFORM sh/ksh
under SunOS (illumos).
For example, it segfaults in configure when trying to build ffmpeg010 on i386 (ABI=32).
I isolated it to a problem in the function 'tenter' found in table.c.
In 'struct table' both size and nfree are 'short', so for grins I first tried
simply 'min' with INT32_MAX then decided on setting size & nsize as unsigned shorts
(avoiding for the moment changing the size of the structure):
> richard@omnis:/home/richard/src/pkgsrc/shells/pdksh$ git diff files/table.c
> diff --git a/shells/pdksh/files/table.c b/shells/pdksh/files/table.c
> index 1ac8fc3..9a9e459 100644
> --- a/shells/pdksh/files/table.c
> +++ b/shells/pdksh/files/table.c
> @@ -7,6 +7,9 @@
> #include "sh.h"
>
> #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 +119,7 @@ tenter(tp, n, h)
> }
>
> if (tp->nfree <= 0) { /* too full */
> - texpand(tp, 2*tp->size);
> + texpand(tp, min(UINT16_MAX,2*tp->size));
> goto Search;
> }
This works fine, but why make it difficult?
I'd simply prefer to patch table.h as follows:
> diff --git a/shells/pdksh/files/table.h b/shells/pdksh/files/table.h
> index 637d1c8..ec20352 100644
> --- a/shells/pdksh/files/table.h
> +++ b/shells/pdksh/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 */
> };
that gives plenty of headroom. (ie leave the errors to the malloc handling part)
It seems to build and run fine as well and rather avoids needing to patch table.c at all.
But I noticed two other things in addition.
1. upstream pdksh-5.2.14-patches.2.gz doesn't seem to be applied, so I took the liberty of applying
this simple patch on exec.c for rv being propagated or not.
BTW, this patch dates back to 2001, no idea why it is neither applied in pkgsrc nor in NetBSD?
2. There are a number of simple patches in NetBSD not applied [yet] in pkgsrc that seem harmless
enough (outside of lint).
So I'd like to propose here that netbsd updates ksh with both the patch as indicated in 1) as well
as changing size and nfree to 'int' as well as putting the guard for INTMAX_MAX as follows:
> richard@omnis:/home/richard/src/netbsd/src/bin/ksh$ git diff --cached
> diff --git a/bin/ksh/exec.c b/bin/ksh/exec.c
> index 93c1aa2..4c7ef7e 100644
> --- a/bin/ksh/exec.c
> +++ b/bin/ksh/exec.c
> @@ -83,6 +83,7 @@ execute(t, flags)
> {
> int i;
> volatile int rv = 0;
> + volatile int rv_prop = 0; /* rv being propogated or newly generated? */
> int pv[2];
> char ** volatile ap;
> char *s, *cp;
> @@ -164,6 +165,7 @@ execute(t, flags)
>
> case TPAREN:
> rv = execute(t->left, flags|XFORK);
> + rv_prop = 1;
> break;
>
> case TPIPE:
> @@ -284,6 +286,7 @@ execute(t, flags)
> rv = execute(t->right, flags & XERROK);
> else
> flags |= XERROK;
> + rv_prop = 1;
> break;
>
> case TBANG:
> @@ -332,6 +335,7 @@ execute(t, flags)
> }
> }
> rv = 0; /* in case of a continue */
> + rv_prop = 1;
> if (t->type == TFOR) {
> while (*ap != NULL) {
> setstr(global(t->str), *ap++, KSH_UNWIND_ERROR);
> @@ -343,6 +347,7 @@ execute(t, flags)
> for (;;) {
> if (!(cp = do_selectargs(ap, is_first))) {
> rv = 1;
> + rv_prop = 0;
> break;
> }
> is_first = FALSE;
> @@ -374,6 +379,7 @@ execute(t, flags)
> rv = 0; /* in case of a continue */
> while ((execute(t->left, XERROK) == 0) == (t->type == TWHILE))
> rv = execute(t->right, flags & XERROK);
> + rv_prop = 1;
> break;
>
> case TIF:
> @@ -383,6 +389,7 @@ execute(t, flags)
> rv = execute(t->left, XERROK) == 0 ?
> execute(t->right->left, flags & XERROK) :
> execute(t->right->right, flags & XERROK);
> + rv_prop = 1;
> break;
>
> case TCASE:
> @@ -395,10 +402,12 @@ execute(t, flags)
> break;
> Found:
> rv = execute(t->left, flags & XERROK);
> + rv_prop = 1;
> break;
>
> case TBRACE:
> rv = execute(t->left, flags & XERROK);
> + rv_prop = 1;
> break;
>
> case TFUNCT:
> @@ -410,6 +419,7 @@ execute(t, flags)
> * (allows "ls -l | time grep foo").
> */
> rv = timex(t, flags & ~XEXEC);
> + rv_prop = 1;
> break;
>
> case TEXEC: /* an eval'd TCOM */
> @@ -437,7 +447,7 @@ execute(t, flags)
> quitenv(); /* restores IO */
> if ((flags&XEXEC))
> unwind(LEXIT); /* exit child */
> - if (rv != 0 && !(flags & XERROK)) {
> + if ((rv != 0) && (rv_prop == 0) && !(flags & XERROK)) {
> if (Flag(FERREXIT))
> unwind(LERROR);
> trapsig(SIGERR_);
> diff --git a/bin/ksh/table.c b/bin/ksh/table.c
> index 025b56d..9bf576b 100644
> --- a/bin/ksh/table.c
> +++ b/bin/ksh/table.c
> @@ -14,6 +14,10 @@ __RCSID("$NetBSD: table.c,v 1.4 2003/06/23 11:39:04 agc Exp $");
>
> #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));
>
> @@ -122,7 +126,7 @@ tenter(tp, n, h)
> }
>
> if (tp->nfree <= 0) { /* too full */
> - texpand(tp, 2*tp->size);
> + texpand(tp, min(INTMAX_MAX, 2*tp->size));
> goto Search;
> }
>
> diff --git a/bin/ksh/table.h b/bin/ksh/table.h
> index 9dc42c8..dcab6d6 100644
> --- a/bin/ksh/table.h
> +++ b/bin/ksh/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 */
> };
>
Once accepted here in NetBSD, I have a patchset bringing the NetBSD patches (excepting lint)
into pkgsrc, including the patches above.
Cheers
--
Richard PALO
Home |
Main Index |
Thread Index |
Old Index