Bug 3639 - Comments on the same line as parameters in smb.conf are not diagnosed or honored
Comments on the same line as parameters in smb.conf are not diagnosed or honored
Status: NEW
Product: Samba 3.0
Classification: Unclassified
Component: Config Files
3.0.21c
All All
: P3 minor
: none
Assigned To: Samba Bugzilla Account
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-03-28 15:42 UTC by David S. Collier-Brown
Modified: 2007-08-19 08:59 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David S. Collier-Brown 2006-03-28 15:42:18 UTC
Gerry Carter noted "Samba does not allow mixing of comment lines and parameters. Be careful not to put comments on the same line as anything else, such as:
path = /data # server's data partition
Errors such as this, where the parameter value is defined with a string, can be tricky to notice. The testparm program won’t complain.  The only clues you’ll receive are that testparm reports the path parameter set to /d # server's data partition, and the failures that result when clients attempt to access the share."

I claim this is a bug in both the parser and testparm.
Comment 1 David S. Collier-Brown 2006-03-28 17:27:38 UTC
A possible proof of concept is:
/*
 * stripcommment -- remove non-quoted # comments from the
 *      right side of text lines. The only quotes supported
 *      are double quotes ("), as single quotes may actually
 *      be apostrophies. As this is specifically for Samba,
 *      strips the \n, as that's already done, instead of
 *      reinserting it.
 */
#include <stdio.h>
#include <stdlib.h>

 char *
stripcomment(char *line) {
        int     quoted = 0;
        char    *p = line;
        int     c;

        for (; (c = *p) != NULL; p++) {
                switch (c) {
                case '"':
                        quoted = !quoted;
                        break;
                case '#':
                        if (quoted) {
                                continue;
                        }
                        else {
                                *p = '\0';
                                return line;
                        }
                case '\n':
                        *p = '\0';
                        return line;
                }
        }
        return line;
}

#ifdef TEST
 void
main() {
        char line[1024];
        while (fgets(line, sizeof(line), stdin) != NULL) {
                printf("%s\n", stripcomment(line));
        }
        exit(0);
}
#endif
Comment 2 Andrew Bartlett 2006-03-28 18:24:44 UTC
I'm unclear, but I think this bug was meant to be directed at Samba3. 

We should sync up the two different loadparm systems however.
Comment 3 David S. Collier-Brown 2006-03-28 21:58:43 UTC
For Samba 3, a tentative fix which parses
 comment = a comment "with a quoted #" and a # comment
as
 comment = a comment "with a quoted #" and a
follows:

froggy> sccs diffs -u params.c

------- params.c -------
--- /tmp/sccs.e9aOPV    Tue Mar 28 22:53:03 2006
+++ params.c    Tue Mar 28 22:51:12 2006
@@ -119,6 +119,10 @@
         /* be sure to return chars >127 as positive values */
        return (int)( *(f->p++) & 0x00FF );
 }
+static void myungetc(myFILE *f, int c) {
+       f->p--;
+       *(f->p) = (char) (c & 0x00FF);
+}

 static void myfile_close(myFILE *f)
 {
@@ -347,6 +351,7 @@
        int   i       = 0;    /* Position within bufr. */
        int   end     = 0;    /* bufr[end] is current end-of-string. */
        int   vstart  = 0;    /* Starting position of the parameter value. */
+       int   doingQuote = False; /* Processing quoted string. */
        const char *func    = "params.c:Parameter() -";

        /* Read the parameter name. */
@@ -422,24 +427,46 @@
                }

                switch(c) {
-                       case '\r': /* Explicitly remove '\r' because the older */
-                               c = mygetc( InFile );   /* version called fgets_slash() which also  */
-                               break;                /* removes them.                   */
+                       /* Explicitly remove '\r' because the older */
+                       /* version called fgets_slash() which also  */
+                       /* removes them.*/
+                       case '\r':
+                               c = mygetc( InFile );
+                               break;

-                       case '\n': /* Marks end of value unless there's a '\'. */
+                       /* A '\n' marks end of value unless there's a '\'. */
+                       case '\n':
                                i = Continuation( bufr, i );
                                if( i < 0 ) {
                                        c = 0;
                                } else {
-                                       for( end = i; (end >= 0) && isspace((int)bufr[end]); end-- )
+                                       for (end = i; (end >= 0) && isspace((int)bufr[end]); end-- )
                                                ;
                                        c = mygetc( InFile );
                                }
                                break;

-                       default: /* All others verbatim.  Note that spaces do not advance <end>.  This allows trimming  */
+                       /* An unquoted # means a comment to EOL. */
+                       case '#':
+                               if (! doingQuote) {
+                                       while (mygetc( InFile ) != '\n')
+                                               ;
+                                       myungetc(InFile, '\n');
+                                       c = '\n';
+                               }
+                               goto default_label;
+
+                       case '"':
+                               doingQuote = ! doingQuote;
+                               /* Fall through into default case. */
+
+                       /* All others verbatim.  Note that spaces do not */
+                       /* advance <end>.  This allows trimming  of */
+                       /*  whitespace at the end of the line. */
+                       default_label: /* got default is not legal... */
+                       default:
                                bufr[i++] = c;
-                               if( !isspace( c ) )  /* of whitespace at the end of the line.     */
+                               if( !isspace( c ) )
                                        end = i;
                                c = mygetc( InFile );
                                break;
@@ -487,7 +514,7 @@

                        case ';': /* Comment line. */
                        case '#':
-                               c = EatComment( InFile );
+                               c = EatComment( InFile  );
                                break;

                        case '[': /* Section Header. */


Some of the above are comment changes so I could read
them, and do not affect the fix.  --dave c-b
Comment 4 David S. Collier-Brown 2006-03-30 16:28:30 UTC
And here's the same fix against samba 4, at revision 14828.
froggy> sccs diffs -u params.c

------- params.c -------
--- /tmp/sccs.ooa4Jf    Thu Mar 30 17:24:12 2006
+++ params.c    Thu Mar 30 17:23:31 2006
@@ -106,6 +106,12 @@
        return (int)( *(f->p++) & 0x00FF );
 }

+static void myungetc(myFILE *f, int c) {
+       f->p--;
+       *(f->p) = (char) (c & 0x00FF);
+}
+
+
 static void myfile_close(myFILE *f)
 {
        talloc_free(f);
@@ -313,6 +319,7 @@
   int   i       = 0;    /* Position within bufr. */
   int   end     = 0;    /* bufr[end] is current end-of-string. */
   int   vstart  = 0;    /* Starting position of the parameter value. */
+  int  doingQuote = False; /* Processing quoted string. */
   const char *func    = "params.c:Parameter() -";

   /* Read the parameter name. */
@@ -419,9 +426,23 @@
           }
         break;

+      case '#':
+       if (! doingQuote) {
+           while (mygetc( InFile ) != '\n')
+               ;
+           myungetc(InFile, '\n');
+           c = '\n';
+       }
+       goto default_label;
+
+      case '"':
+       doingQuote = ! doingQuote;
+       /* Fall through into default case. */
+
       default:               /* All others verbatim.  Note that spaces do */
-        InFile->bufr[i++] = c;       /* not advance <end>.  This allows trimming  */
-        if( !isspace( c ) )  /* of whitespace at the end of the line.     */
+      default_label:         /* not advance <end>.  This allows trimming  */
+        InFile->bufr[i++] = c; /* of whitespace at the end of the line.   */
+        if( !isspace( c ) )
           end = i;
         c = mygetc( InFile );
         break;
Comment 5 David S. Collier-Brown 2007-08-18 14:45:54 UTC
The following patch for Samba 3, revision 24545, implements comments
on the same line as parameters: this smb.conf...

[global]
        case sensitive = no # overrides auto
        workgroup = "FROGGY"
        server string = "test version # 3"
        passwd chat debug = yes

returns the following from testparm...
Press enter to see a dump of your service definitions

[global]
        workgroup = FROGGY
        server string = test version # 3
        passwd chat debug = Yes
        case sensitive = No

...

froggy> cat params.diff
--- params.old.c        Fri Aug 10 09:56:55 2007
+++ params.c    Sat Aug 18 15:37:38 2007
@@ -346,6 +346,7 @@
        int   i       = 0;    /* Position within bufr. */
        int   end     = 0;    /* bufr[end] is current end-of-string. */
        int   vstart  = 0;    /* Starting position of the parameter value. */
+       int   doingQuote = False; /* Processing quoted string. */
        const char *func    = "params.c:Parameter() -";

        /* Read the parameter name. */
@@ -391,7 +392,6 @@
                                bufr[i] = '\0';
                                DEBUG(1,("%s Unexpected end-of-file at: %s\n", func, bufr ));
                                return True;
-
                        default:
                                if(isspace( c )) {
                                        /* One ' ' per whitespace region. */
@@ -436,7 +436,20 @@
                                }
                                break;

+                       case '"':
+                               doingQuote = ! doingQuote;
+                               c = mygetc( InFile );
+                               break;
+
+                       case '#': /* If unquoted, skip to end. */
+                               if (! doingQuote) {
+                                       c = EatComment( InFile );
+                                       break;
+                               }
+                               /* Fall into default case. */
+
                        default: /* All others verbatim.  Note that spaces do not advance <end>.  This allows trimming  */
+                       default_label: /* Target for the above goto. */
                                bufr[i++] = c;
                                if( !isspace( c ) )  /* of whitespace at the end of the line.     */
                                        end = i;

Samba 4 and the announcement to follow, probably tomorrow.
Comment 6 David S. Collier-Brown 2007-08-19 08:59:26 UTC
Here's Samab 4, at revision 24545.

From this smb.conf file:
---
[global]
        workgroup = "FROGGY" # The quotes should be ignored
        server string = "test version # 3"
---
we get this testparm output
Loaded services file OK.
Press enter to see a dump of your service definitions

# Global parameters
[global]
        workgroup = FROGGY
        server string = test version # 3
...
[IPC$]
        comment = IPC Service (test version # 3)
        path = /tmp
---
and the diff is:
---
--- params_4.old.c      Sat Aug 18 15:47:51 2007
+++ params_4.c  Sun Aug 19 09:54:26 2007
@@ -312,6 +312,7 @@
   int   i       = 0;    /* Position within bufr. */
   int   end     = 0;    /* bufr[end] is current end-of-string. */
   int   vstart  = 0;    /* Starting position of the parameter value. */
+  int   doingQuote = False; /* Processing quoted string. */
   const char *func    = "params.c:Parameter() -";

   /* Read the parameter name. */
@@ -417,7 +418,18 @@
           c = mygetc( InFile );
           }
         break;
+      case '"':
+        doingQuote = ! doingQuote;
+        c = mygetc( InFile );
+        break;

+      case '#': /* If unquoted, skip to end. */
+        if (! doingQuote) {
+           c = EatComment( InFile );
+           break;
+        }
+        /* Fall into default case. */
+
       default:               /* All others verbatim.  Note that spaces do */
         InFile->bufr[i++] = c;       /* not advance <end>.  This allows trimming  */
         if( !isspace( c ) )  /* of whitespace at the end of the line.     */