Re: [PATCH] find RLIM_NLIMITS correctly on Cygwin

[Daniel Shahaf <d.s@daniel.shahaf.name>]

[-- Attachment #1: Type: text/plain, Size: 2489 bytes --]

Continuing my review:

Jun T wrote on Tue, 25 Feb 2020 18:38 +0900:
> --- a/Src/Builtins/rlimits.awk
> +++ /dev/null
> @@ -1,116 +0,0 @@
> -#
> -# rlimits.awk: {g,n}awk script to generate rlimits.h
> -#
> -# NB: On SunOS 4.1.3 - user-functions don't work properly, also \" problems
> -# Without 0 + hacks some nawks compare numbers as strings
> -#
> -BEGIN {limidx = 0}
> -
> -/^[\t ]*(#[\t ]*define[\t _]*RLIMIT_[A-Z_]*[\t ]*[0-9][0-9]*|RLIMIT_[A-Z_]*,[\t ]*|_*RLIMIT_[A-Z_]*[\t ]*=[\t ]*[0-9][0-9]*,[\t ]*)/ {
⋮
> -	    if (limnam == "RSS")     { msg[limnum] = "Mresident" }
⋮
> -END {
> -    if (limrev["MEMLOCK"] != "") {
> -        irss = limrev["RSS"]
> -        msg[irss] = "Mmemoryuse"
> -    }  

Question.  I compared the output before and after the patch and I see
the following difference:

    % diff -U0 =(zsh-5.7.1 -fc 'limit') =(limit)
    --- /tmp/zshZXxkUD      2020-03-20 18:00:04.239999929 +0000
    +++ /tmp/zshxTTscg      2020-03-20 18:00:04.239999929 +0000
    @@ -6 +6 @@
    -memoryuse       unlimited
    +resident        unlimited
    zsh: exit 1     diff -U0 =(zsh-5.7.1 -fc 'limit') =(limit)

It seems to be caused by the C implementation not having an equivalent
of the above piece of code. this difference intentional?

> +++ b/Src/Builtins/rlimits.c
> @@ -53,11 +40,214 @@ enum {
> +/* table of known resources */
> +static resinfo_t known_resources[] = {
> +    {RLIMIT_CPU, "cputime", ZLIMTYPE_TIME, 1,
> +		't', "cpu time (seconds)"},
> +    {RLIMIT_FSIZE, "filesize", ZLIMTYPE_MEMORY, 512,
> +		'f', "file size (blocks)"},
> ⋮

What will happen if two different elements of this array have the same
option letter?

When I simulate that (by manually changing the «'f'» to «'t'»), I get
output such as:
.
    % b/Src/zsh -fc 'ulimit -a'
    -t: cpu time (seconds)              unlimited
    ⋮
    -t: file size (blocks)              unlimited
.
Given this output, people are liable to invoke «ulimit -t» in an
attempt to change the file size limit.

Currently, each of the letters [mrvx] is used by two different elements.
I haven't checked whether both elements of any of these pairs can be
present on a single system, but in any case, more collisions may be
added in the future.

Therefore, I was wondering if we should have a test for this situation,
or possibly a runtime check; see the attached series for example.

Sorry for my slow response.

Cheers,

Daniel

[-- Attachment #2: 0001-zsh-rlimits-Make-known_resources-const-in-set_re.patch.txt --]
[-- Type: text/plain, Size: 2039 bytes --]

From 0f3dc2ee15c6bb1a005728d3d1107cd4da6e0c7f Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Fri, 20 Mar 2020 18:48:28 +0000
Subject: [PATCH 1/2] zsh/rlimits: Make known_resources const in set_resinfo()
 but not elsewhere.

---
 Src/Builtins/rlimits.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/Src/Builtins/rlimits.c b/Src/Builtins/rlimits.c
index aa9b9dd48..c3c031fff 100644
--- a/Src/Builtins/rlimits.c
+++ b/Src/Builtins/rlimits.c
@@ -49,8 +49,13 @@ typedef struct resinfo_T {
     char* descr;	/* used by ulimit builtin */
 } resinfo_T;
 
-/* table of known resources */
-static const resinfo_T known_resources[] = {
+/* table of known resources
+ *
+ * Not const since set_resinfo() may change some of the letters to 'N' in case
+ * of collisions.  However, all access should be through the "resinfo" global,
+ * which exposes this as a const array.
+ */
+static resinfo_T known_resources[] = {
     {RLIMIT_CPU, "cputime", ZLIMTYPE_TIME, 1,
 		't', "cpu time (seconds)"},
     {RLIMIT_FSIZE, "filesize", ZLIMTYPE_MEMORY, 512,
@@ -175,14 +180,15 @@ static void
 set_resinfo(void)
 {
     int i;
+    resinfo_T **resinfo_mutable;
 
-    resinfo = (const resinfo_T **)zshcalloc(RLIM_NLIMITS*sizeof(resinfo_T *));
+    resinfo_mutable = (resinfo_T **)zshcalloc(RLIM_NLIMITS*sizeof(resinfo_T *));
 
     for (i=0; i<sizeof(known_resources)/sizeof(resinfo_T); ++i) {
-	resinfo[known_resources[i].res] = &known_resources[i];
+	resinfo_mutable[known_resources[i].res] = &known_resources[i];
     }
     for (i=0; i<RLIM_NLIMITS; ++i) {
-	if (!resinfo[i]) {
+	if (!resinfo_mutable[i]) {
 	    /* unknown resource */
 	    resinfo_T *info = (resinfo_T *)zshcalloc(sizeof(resinfo_T));
 	    char *buf = (char *)zalloc(12);
@@ -193,9 +199,11 @@ set_resinfo(void)
 	    info->unit = 1;
 	    info->opt = 'N';
 	    info->descr = buf;
-	    resinfo[i] = info;
+	    resinfo_mutable[i] = info;
 	}
     }
+
+    resinfo = (const resinfo_T **) resinfo_mutable;
 }
 
 /**/

[-- Attachment #3: 0002-zsh-rlimits-Ensure-option-letters-are-unambiguou.patch.txt --]
[-- Type: text/plain, Size: 2001 bytes --]

From 5ffb506e43cd8dd5ecd1945a93e854f1b735cfd5 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Fri, 20 Mar 2020 18:49:42 +0000
Subject: [PATCH 2/2] zsh/rlimits: Ensure option letters are unambiguous at
 runtime.

---
 Src/Builtins/rlimits.c | 15 +++++++++++++++
 Test/B12limit.ztst     | 10 ++++++++++
 2 files changed, 25 insertions(+)

diff --git a/Src/Builtins/rlimits.c b/Src/Builtins/rlimits.c
index c3c031fff..5c260e7db 100644
--- a/Src/Builtins/rlimits.c
+++ b/Src/Builtins/rlimits.c
@@ -181,11 +181,26 @@ set_resinfo(void)
 {
     int i;
     resinfo_T **resinfo_mutable;
+    char seen_letters[sizeof(known_resources)/sizeof(resinfo_T) + 1] = {0};
+    char *seen_p = seen_letters;
 
     resinfo_mutable = (resinfo_T **)zshcalloc(RLIM_NLIMITS*sizeof(resinfo_T *));
 
     for (i=0; i<sizeof(known_resources)/sizeof(resinfo_T); ++i) {
 	resinfo_mutable[known_resources[i].res] = &known_resources[i];
+
+	/* 
+	 * If this resource's option letter is already used, change its option
+	 * letter to 'N'.
+	 */
+	{
+	    char *current_letter = &known_resources[i].opt;
+	    if (*current_letter != 'N' && strchr(seen_letters, *current_letter)) {
+		DPUTS1(1, "duplicate ulimit option letter: %c", *current_letter);
+		*current_letter = 'N';
+	    }
+	    *seen_p++ = *current_letter;
+	}
     }
     for (i=0; i<RLIM_NLIMITS; ++i) {
 	if (!resinfo_mutable[i]) {
diff --git a/Test/B12limit.ztst b/Test/B12limit.ztst
index 5dd7afdbe..922751369 100644
--- a/Test/B12limit.ztst
+++ b/Test/B12limit.ztst
@@ -8,3 +8,13 @@
 F:A failure here does not indicate any error in zsh. It just means there
 F:is a resource in your system that is unknown to zsh developers. Please
 F:report this to zsh-workers mailing list.
+
+  () {
+    set -- ${(f)"$(ulimit -a)"}
+    set -- ${@%%:*}
+    typeset -aU unique_options=( "$@" )
+    # The value of $unique_options is, e.g., ( -t -f '-N 2' -s ... ).
+    (( $# == $#unique_options ))
+  }
+0:check if limit option letters are unique
+