Changeset 0860d9c

Timestamp:

Oct 5, 2023, 4:17:14 PM (17 months ago)

Author:

Michael Brooks <mlbrooks@…>

Branches:

master

Children:

4d860ea3

Parents:

b67b632

Message:

Fix read-to-variable-length-string cases when internal buffer fills.

Also fix read-to-cstring ability to give no-exception cases when an entire buffer fills.

The added test cases run, and fail, when run against prior libcfa.
Doing so illustrates a CFA-string-level bug.
Doing so illustrates a C-string-level changed semantics.

At the CFA-string level, the bug was, when reading strings of just the right length,
what should be two reads ("abc" then "def") gets mashed into one ("abcdef").
These cases are clearly bugs because a test program that just echoes chuncks of delimeted input would do so inaccurately.
They're just hard to drive because the relevant chunk lengths are implementation-dependent, and sometimes big.

At the C-string level, the semantic change concerns when to throw the cstring_length exception.
By this change, make the original semantics,
"An exception means the maximum number of characters was read," into
"An exception means that if the buffer were larger, then more characters would have been read."

The added test cases cover the respective stop conditions for manipulator state "%s", include, exclude, getline, and getline/delimiter.

Files:

• libcfa/src/collections/string_res.cfa (modified) (3 diffs)
• libcfa/src/iostream.cfa (modified) (4 diffs)
• tests/collections/.expect/string-istream-manip.txt (modified) (1 diff)
• tests/collections/.in/string-istream-manip.txt (modified) (1 diff)
• tests/collections/string-istream-manip.cfa (modified) (2 diffs)
• tests/io/.expect/manipulatorsInput.arm64.txt (modified) (1 diff)
• tests/io/.expect/manipulatorsInput.x64.txt (modified) (1 diff)
• tests/io/.expect/manipulatorsInput.x86.txt (modified) (1 diff)
• tests/io/.in/manipulatorsInput.txt (modified) (1 diff)
• tests/io/manipulatorsInput.cfa (modified) (1 diff)

libcfa/src/collections/string_res.cfa ¶

@@ -218 +218 @@
     // Read in chunks.  Often, one chunk is enough.  Keep the string that accumulates chunks last in the heap,
     // so available room is rest of heap.  When a chunk fills the heap, force growth then take the next chunk.
-    for (;;) {
+    for (bool cont = true; cont; ) {
+        cont = false;
+
         // Append dummy content to temp, forcing expansion when applicable (occurs always on subsequent loops)
         // length 2 ensures room for at least one real char, plus scanf/pipe-cstr's null terminator
@@ -228 +230 @@
         temp.Handle.ulink->EndVbyte -= 2;
 
-        // rest of heap, less 1 byte for null terminator, is available to read into
-        int lenReadable = (char*)temp.Handle.ulink->ExtVbyte - temp.Handle.ulink->EndVbyte - 1;
-        assert (lenReadable >= 1);
+        // rest of heap is available to read into
+        int lenReadable = (char*)temp.Handle.ulink->ExtVbyte - temp.Handle.ulink->EndVbyte;
+        assert (lenReadable >= 2);
 
         // get bytes
-        in | wdi( lenReadable + 1, lenReadable, temp.Handle.ulink->EndVbyte );
+        try {
+            in | wdi( lenReadable, temp.Handle.ulink->EndVbyte );
+        } catch (cstring_length*) {
+            cont = true;
+        }
         int lenWasRead = strlen(temp.Handle.ulink->EndVbyte);
 
@@ -239 +245 @@
         temp.Handle.lnth += lenWasRead;
         temp.Handle.ulink->EndVbyte += lenWasRead;
-
-      if (lenWasRead < lenReadable) break;
     }
 

libcfa/src/iostream.cfa ¶

@@ -22 +22 @@
 #include <float.h>                                                                              // DBL_DIG, LDBL_DIG
 #include <complex.h>                                                                    // creal, cimag
+#include <ctype.h>                                                                              // isspace
 //#include <stdio.h>
 
@@ -29 +30 @@
 extern char *strcpy (char *__restrict __dest, const char *__restrict __src) __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__nonnull__ (1, 2)));
 extern void *memcpy (void *__restrict __dest, const void *__restrict __src, size_t __n) __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__nonnull__ (1, 2)));
+extern char *strchr(const char *str, int ch);
 } // extern "C"
 
@@ -991 +993 @@
                         // wd is buffer bytes available (for input chars + null terminator)
                         // rwd is count of input chars
-                        int rwd = f.flags.rwd ? f.wd : (f.wd - 1);
+                        int rwd;
+                        if (f.flags.rwd) {
+                                verify (f.wd >= 0);
+                                rwd = f.wd;
+                        } else {
+                                verify (f.wd >= 1);
+                                rwd = f.wd - 1;
+                        } // if
                         start += sprintf( &fmtstr[start], "%d", rwd );
                 }
@@ -1018 +1027 @@
                 //fprintf( stderr, "KK %s %zd %d %c %s\n", fmtstr, len, check, f.s[check], f.s );
 
-                if (! f.flags.ignore && ! f.flags.rwd && f.s[check] != '\0' ) // sentinel overwritten ?
-                        throw (cstring_length){ &cstring_length_vt };
+                if ( ! f.flags.ignore && ! f.flags.rwd && f.s[check] != '\0' ) { // sentinel overwritten ?
+                        // buffer filled, but would we have kept going?
+                        if ( ! eof( is ) ) {
+                                char peek;
+                                fmt( is, "%c", &peek );
+                                ungetc( is, peek );
+                                bool hasMore;
+                                if (f.flags.delimiter) { // getline
+                                        hasMore = (peek != f.delimiter[0]);
+                                } else if (f.scanset) { // incl/excl
+                                        bool peekMatch = strchr(f.scanset, peek) != 0p;
+                                        hasMore = f.flags.inex ? (!peekMatch) : (peekMatch);
+                                } else { // %s
+                                        hasMore = !isspace(peek);
+                                }
+                                if (hasMore) throw (cstring_length){ &cstring_length_vt };
+                        } // if
+                } // if
 
                 if ( f.flags.delimiter ) {                                              // getline ?

tests/collections/.expect/string-istream-manip.txt ¶

@@ +1 @@
+preS1 0123456
+preS1 x
+preS2 01234567
+preS2 x
+preS3 012345678
+preS3 x
+preS4 0123456789
+preS4 x
+preSMN1 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456
+preSMN1 x
+preSMN2 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...01234567
+preSMN2 x
+preSMN3 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...012345678
+preSMN3 x
+preSMN4 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456789
+preSMN4 x
+preRMN1 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456
+preRMN1 x
+preRMN2 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...01234567
+preRMN2 x
+preRMN3 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...012345678
+preRMN3 x
+preRMN4 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456789
+preRMN4 x
+preSMI1 "...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...       "
+preSMI1 "x"
+preSMI2 "...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...        "
+preSMI2 "x"
+preSMI3 "...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...         "
+preSMI3 "x"
+preSMI4 "...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...          "
+preSMI4 "x"
+preSME1 "...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...       "
+preSME1 "x"
+preSME2 "...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...        "
+preSME2 "x"
+preSME3 "...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...         "
+preSME3 "x"
+preSME4 "...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...          "
+preSME4 "x"
+preSMG1 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456
+preSMG1 x
+preSMG2 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...01234567
+preSMG2 x
+preSMG3 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...012345678
+preSMG3 x
+preSMG4 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456789
+preSMG4 x
+preSMD1 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456
+preSMD1 x
+preSMD2 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...01234567
+preSMD2 x
+preSMD3 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...012345678
+preSMD3 x
+preSMD4 ...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456789
+preSMD4 x
 1 yyyyyyyyyyyyyyyyyyyy
 2 abcxxx

tests/collections/.in/string-istream-manip.txt ¶

@@ +1 @@
+0123456 x
+01234567 x
+012345678 x
+0123456789 x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456 x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...01234567 x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...012345678 x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456789 x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456 x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...01234567 x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...012345678 x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456789 x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...       -x-
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...        -x-
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...         -x-
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...          -x-
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...       -x-
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...        -x-
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...         -x-
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...          -x-
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456
+x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...01234567
+x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...012345678
+x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456789
+x
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456@x@
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...01234567@x@
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...012345678@x@
+...:...:...:...|...:...:...:...|...:...:...:...|...:...:...:...#...:...:...:...|...:...:...:...|...:...:...:...|...:...0123456789@x@
 abc 
 cccccb 

tests/collections/string-istream-manip.cfa ¶

@@ -3 +3 @@
 #include <collections/string.hfa>
 #include <collections/string_res.hfa>
+#include <stdio.h>
+
+// No-op manipulators.
+// Temporary hack while there are two code paths in the string implementation.
+// (One for reading plain strings, the other for reading via a manipulator.)
+// The test cases that use plainjane(-) are exercising the via-manipulator code path,
+// just with trivial manipulation.
+static _Istream_Sstr plainjane( string     & s )  { return (_Istream_Sstr)@{  s, {{0p}, -1, {.flags.rwd : false}} }; }
+static _Istream_Rstr plainjane( string_res & s )  { return (_Istream_Rstr)@{ &s, {{0p}, -1, {.flags.rwd : false}} }; }
+
+static void forceStringHeapFreeSpaceTo(int desiredSize) {
+    for (1_000_000) {
+        string x = "a";
+        (void)x;
+      if (desiredSize == DEBUG_string_bytes_avail_until_gc(DEBUG_string_heap())) return;
+    }
+    sout | "Unable to force size" | desiredSize | "in 1,000,000 tries";
+}
 
 int main() {
+    // These "pre" cases deal with issues analogous to the "pre" cases of io/manipulatorsInput.
+    // The acceptance criterion is simpler but driving the cases is harder.
+    // The tests just read strings and echo what they read; acceptance of simple echoing assures
+    // no spurious splitting merging.
+    // The lengths of the strings are chosen to match white-box knowledge of when the string layer
+    // has tor drive the cstring layer through a second iteration:
+    //  - for no-manip, lengths are near the room at end of string heap
+    //    (chosen target size of 9 showed the original bug on preS2, aligned with the other cases)
+    //  - for manip, lengths are near the auxiliary buffer size of 128
+    // Only first case repeats for string_res; rest run only from the passthru string layer.
+    // Similarly, the manipulator breadth isn't checked at the cstring layer either.
+    {
+        // S: string, no manipulator
+        void echoTillX(const char * casename) {
+            string s;
+            do {
+                forceStringHeapFreeSpaceTo(9);
+                sin | s;
+                sout | casename | s;
+            } while ( size(s) > 0 && s[size(s)-1] != 'x' );
+        }
+        echoTillX("preS1");
+        echoTillX("preS2");
+        echoTillX("preS3");
+        echoTillX("preS4");
+    }
+    {
+        // SMN: string, manipulator for no-op
+        void echoTillX(const char * casename) {
+            string s;
+            do {
+                sin | plainjane( s );
+                sout | casename | s;
+            } while ( size(s) > 0 && s[size(s)-1] != 'x' );
+        }
+        echoTillX("preSMN1");
+        echoTillX("preSMN2");
+        echoTillX("preSMN3");
+        echoTillX("preSMN4");
+    }
+    {
+        // RMN: string_res, manipulator for no-op
+        void echoTillX(const char * casename) {
+            string_res s;
+            do {
+                sin | plainjane( s );
+                sout | casename | s;
+            } while ( size(s) > 0 && s[size(s)-1] != 'x' );
+        }
+        echoTillX("preRMN1");
+        echoTillX("preRMN2");
+        echoTillX("preRMN3");
+        echoTillX("preRMN4");
+    }
+    {
+        // SMI: string, manipulator `incl`
+        void echoTillX(const char * casename) {
+            string s;
+            do {
+                sin | skip("-\n");
+                sin | incl( ".:|# x", s );
+                sout | casename | " \"" | s | "\"";
+            } while ( size(s) > 0 && s[size(s)-1] != 'x' );
+        }
+        echoTillX("preSMI1");
+        echoTillX("preSMI2");
+        echoTillX("preSMI3");
+        echoTillX("preSMI4");
+    }
+    {
+        // SME: string, manipulator `excl`
+        void echoTillX(const char * casename) {
+            string s;
+            do {
+                sin | skip("-\n");
+                sin | excl( "-\n", s );
+                sout | casename | " \"" | s | "\"";
+            } while ( size(s) > 0 && s[size(s)-1] != 'x' );
+        }
+        echoTillX("preSME1");
+        echoTillX("preSME2");
+        echoTillX("preSME3");
+        echoTillX("preSME4");
+    }
+    sin | skip("-\n");
+    {
+        // SMG: string, manipulator `getline`
+        void echoTillX(const char * casename) {
+            string s;
+            do {
+                sin | getline( s );
+                sout | casename | s;
+            } while ( size(s) > 0 && s[size(s)-1] != 'x' );
+        }
+        echoTillX("preSMG1");
+        echoTillX("preSMG2");
+        echoTillX("preSMG3");
+        echoTillX("preSMG4");
+    }
+    {
+        // SMD: string, manipulator (`getline` with custom) delimiter
+        void echoTillX(const char * casename) {
+            string s;
+            do {
+                sin | getline( s, '@' );
+                sout | casename | s;
+            } while ( size(s) > 0 && s[size(s)-1] != 'x' );
+            sin | skip(" \n");
+        }
+        echoTillX("preSMD1");
+        echoTillX("preSMD2");
+        echoTillX("preSMD3");
+        echoTillX("preSMD4");
+    }
+
     /* Keep harmonized with io/manipulatorsInput */
     {
@@ -31 +164 @@
                 sin | "\n";
         }
+    // Full repeat on string_res layer assures the full manipulator vocabulary is supported there.
     {
         string_res s = "yyyyyyyyyyyyyyyyyyyy";

tests/io/.expect/manipulatorsInput.arm64.txt ¶

@@ -1 +1 @@
 pre1 "123456", canary ok
-pre2a "1234567", exception occurred, canary ok
-pre2b "89", canary ok
+pre2 "1234567", canary ok
+pre3a "1234567", exception occurred, canary ok
+pre3b "8", canary ok
+pre4a "1234567", exception occurred, canary ok
+pre4b "89", canary ok
 1 yyyyyyyyyyyyyyyyyyyy
 2 abcxxx

tests/io/.expect/manipulatorsInput.x64.txt ¶

@@ -1 +1 @@
 pre1 "123456", canary ok
-pre2a "1234567", exception occurred, canary ok
-pre2b "89", canary ok
+pre2 "1234567", canary ok
+pre3a "1234567", exception occurred, canary ok
+pre3b "8", canary ok
+pre4a "1234567", exception occurred, canary ok
+pre4b "89", canary ok
 1 yyyyyyyyyyyyyyyyyyyy
 2 abcxxx

tests/io/.expect/manipulatorsInput.x86.txt ¶

@@ -1 +1 @@
 pre1 "123456", canary ok
-pre2a "1234567", exception occurred, canary ok
-pre2b "89", canary ok
+pre2 "1234567", canary ok
+pre3a "1234567", exception occurred, canary ok
+pre3b "8", canary ok
+pre4a "1234567", exception occurred, canary ok
+pre4b "89", canary ok
 1 yyyyyyyyyyyyyyyyyyyy
 2 abcxxx

tests/io/.in/manipulatorsInput.txt ¶

@@ -1 +1 @@
 123456
+1234567
+12345678
 123456789
 abc 

tests/io/manipulatorsInput.cfa ¶

@@ -45 +45 @@
                 }
 
-                rep("pre1");
-                rep("pre2a");
-                rep("pre2b");
+                rep("pre1");    // 123456     |  123456
+                rep("pre2");    // 1234567    |  1234567
+                rep("pre3a");   // 12345678   |  1234567
+                rep("pre3b");   //            |  8
+                rep("pre4a");   // 123456789  |  1234567
+                rep("pre4b");   //            |  89
+
                 scanf("\n");  // next test does not start with %s so does not tolerate leading whitespace
         }