Heap overflow with empty global pawndoc comments

[Y-Less]

Issue description:

If you have a lot of stand-alone:

///

Eventually the compiler can crash. This is because each time a single extra space is added to the global pawndoc string, but that isn't included in the length calculation. The code actually tries to account for this space:

      if (length>0)
        length++;   /* count 1 extra for a separating space */

But as you can see, only if there was some text previously. As this comment is on its own, there isn't and the addition never gets done.

Minimal complete verifiable example (MCVE):

I'm not doing a repro as this only happens when I compile the entire YSI test script with fixes.inc and more - a huge amount of code. Less than that doesn't overflow the buffer enough to affect anything, so the "minimal" repro would be huge. Fortunately, I already found and fixed the issue, this is just here for reference.

[Y-Less]

 source/compiler/sc1.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/source/compiler/sc1.c b/source/compiler/sc1.c
index a50409e..308da67 100644
--- a/source/compiler/sc1.c
+++ b/source/compiler/sc1.c
@@ -1982,25 +1982,30 @@ void sc_attachdocumentation(symbol *sym)
   /* first check the size */
   length=0;
   for (line=0; (str=get_docstring(line))!=NULL && *str!=sDOCSEP; line++) {
-    if (length>0)
-      length++;   /* count 1 extra for a separating space */
-    length+=strlen(str);
+    if (str[0]!='\0') {
+      if (length>0)
+        length++;   /* count 1 extra for a separating space */
+      length+=strlen(str);
+    }
   } /* for */
-  if (sym==NULL && sc_documentation!=NULL) {
-    length += strlen(sc_documentation) + 1 + 4; /* plus 4 for "<p/>" */
-    assert(length>strlen(sc_documentation));
-  } /* if */
-
   if (length>0) {
-    /* allocate memory for the documentation */
-    if (sym!=NULL && sym->documentation!=NULL)
+    if (sym==NULL && sc_documentation!=NULL) {
+      length += strlen(sc_documentation) + 1 + 4; /* plus 4 for "<p/>" */
+      assert(length > strlen(sc_documentation));
+    } else if (sym!=NULL && sym->documentation!=NULL) {
       length+=strlen(sym->documentation) + 1 + 4;/* plus 4 for "<p/>" */
+      assert(length > strlen(sym->documentation));
+    } /* if */
+
+    /* allocate memory for the documentation */
     doc=(char*)malloc((length+1)*sizeof(char));
     if (doc!=NULL) {
       /* initialize string or concatenate */
       if (sym==NULL && sc_documentation!=NULL) {
         strcpy(doc,sc_documentation);
         strcat(doc,"<p/>");
+        free(sc_documentation);
+        sc_documentation=NULL;
       } else if (sym!=NULL && sym->documentation!=NULL) {
         strcpy(doc,sym->documentation);
         strcat(doc,"<p/>");
@@ -2011,9 +2016,11 @@ void sc_attachdocumentation(symbol *sym)
       } /* if */
       /* collect all documentation */
       while ((str=get_docstring(0))!=NULL && *str!=sDOCSEP) {
-        if (doc[0]!='\0')
-          strcat(doc," ");
-        strcat(doc,str);
+        if (str[0]!='\0') {
+          if (doc[0]!='\0')
+            strcat(doc," ");
+          strcat(doc,str);
+        }
         delete_docstring(0);
       } /* while */
       if (str!=NULL) {
@@ -2021,12 +2028,12 @@ void sc_attachdocumentation(symbol *sym)
         assert(*str==sDOCSEP);
         delete_docstring(0);
       } /* if */
-      if (sym!=NULL) {
+      if (sym==NULL) {
+        assert(sc_documentation==NULL);
+        sc_documentation=doc;
+      } else {
         assert(sym->documentation==NULL);
         sym->documentation=doc;
-      } else {
-        free(sc_documentation);
-        sc_documentation=doc;
       } /* if */
     } /* if */
   } else {

[Y-Less]

That patch fixes this bug and also slightly more unifies the code between function and global pawndoc. Like why was the length modification for global comments done outside the length > 0 check, but local comments was inside? The latter makes more sense for both.

[Y-Less]

Leaving this open until I merge.