patch 8.1.0544: setting 'filetype' in a modeline causes an error

Problem:    Setting 'filetype' in a modeline causes an error (Hirohito
            Higashi).
Solution:   Don't add the P_INSECURE flag when setting 'filetype' from a
            modeline.  Also for 'syntax'.

Author: brammool

src/option.c

@@ -3284,7 +3284,7 @@ static char *(p_scl_values[]) = {"yes", "no", "auto", NULL};
 static void set_options_default(int opt_flags);
 static void set_string_default_esc(char *name, char_u *val, int escape);
 static char_u *term_bg_default(void);
-static void did_set_option(int opt_idx, int opt_flags, int new_value);
+static void did_set_option(int opt_idx, int opt_flags, int new_value, int value_checked);
 static char_u *option_expand(int opt_idx, char_u *val);
 static void didset_options(void);
 static void didset_options2(void);
@@ -3295,7 +3295,7 @@ static long_u *insecure_flag(int opt_idx, int opt_flags);
 # define insecure_flag(opt_idx, opt_flags) (&options[opt_idx].flags)
 #endif
 static void set_string_option_global(int opt_idx, char_u **varp);
-static char_u *did_set_string_option(int opt_idx, char_u **varp, int new_value_alloced, char_u *oldval, char_u *errbuf, int opt_flags);
+static char_u *did_set_string_option(int opt_idx, char_u **varp, int new_value_alloced, char_u *oldval, char_u *errbuf, int opt_flags, int *value_checked);
 static char_u *set_chars_option(char_u **varp);
 #ifdef FEAT_CLIPBOARD
 static char_u *check_clipboard_option(void);
@@ -4706,6 +4706,7 @@ do_set(
 	    else
 	    {
 		int value_is_replaced = !prepending && !adding && !removing;
+		int value_checked = FALSE;
 
 		if (flags & P_BOOL)		    /* boolean */
 		{
@@ -5236,7 +5237,8 @@ do_set(
 			    // or 'filetype' autocommands may be triggered that can
 			    // cause havoc.
 			    errmsg = did_set_string_option(opt_idx, (char_u **)varp,
-				    new_value_alloced, oldval, errbuf, opt_flags);
+				    new_value_alloced, oldval, errbuf,
+				    opt_flags, &value_checked);
 
 			    if (did_inc_secure)
 				--secure;
@@ -5280,7 +5282,8 @@ do_set(
 		}
 
 		if (opt_idx >= 0)
-		    did_set_option(opt_idx, opt_flags, value_is_replaced);
+		    did_set_option(
+			 opt_idx, opt_flags, value_is_replaced, value_checked);
 	    }
 
 skip:
@@ -5348,8 +5351,10 @@ do_set(
     static void
 did_set_option(
     int	    opt_idx,
-    int	    opt_flags,	    /* possibly with OPT_MODELINE */
-    int	    new_value)	    /* value was replaced completely */
+    int	    opt_flags,	    // possibly with OPT_MODELINE
+    int	    new_value,	    // value was replaced completely
+    int	    value_checked)  // value was checked to be safe, no need to set the
+			    // P_INSECURE flag.
 {
     long_u	*p;
 
@@ -5359,11 +5364,11 @@ did_set_option(
      * set the P_INSECURE flag.  Otherwise, if a new value is stored reset the
      * flag. */
     p = insecure_flag(opt_idx, opt_flags);
-    if (secure
+    if (!value_checked && (secure
 #ifdef HAVE_SANDBOX
 	    || sandbox != 0
 #endif
-	    || (opt_flags & OPT_MODELINE))
+	    || (opt_flags & OPT_MODELINE)))
 	*p = *p | P_INSECURE;
     else if (new_value)
 	*p = *p & ~P_INSECURE;
@@ -6036,6 +6041,7 @@ set_string_option(
     char_u	*saved_newval = NULL;
 #endif
     char_u	*r = NULL;
+    int		value_checked = FALSE;
 
     if (options[opt_idx].var == NULL)	/* don't set hidden option */
 	return NULL;
@@ -6063,8 +6069,8 @@ set_string_option(
 	}
 #endif
 	if ((r = did_set_string_option(opt_idx, varp, TRUE, oldval, NULL,
-							   opt_flags)) == NULL)
-	    did_set_option(opt_idx, opt_flags, TRUE);
+					   opt_flags, &value_checked)) == NULL)
+	    did_set_option(opt_idx, opt_flags, TRUE, value_checked);
 
 #if defined(FEAT_EVAL)
 	/* call autocommand after handling side effects */
@@ -6099,12 +6105,14 @@ valid_filetype(char_u *val)
  */
     static char_u *
 did_set_string_option(
-    int		opt_idx,		/* index in options[] table */
-    char_u	**varp,			/* pointer to the option variable */
-    int		new_value_alloced,	/* new value was allocated */
-    char_u	*oldval,		/* previous value of the option */
-    char_u	*errbuf,		/* buffer for errors, or NULL */
-    int		opt_flags)		/* OPT_LOCAL and/or OPT_GLOBAL */
+    int		opt_idx,		// index in options[] table
+    char_u	**varp,			// pointer to the option variable
+    int		new_value_alloced,	// new value was allocated
+    char_u	*oldval,		// previous value of the option
+    char_u	*errbuf,		// buffer for errors, or NULL
+    int		opt_flags,		// OPT_LOCAL and/or OPT_GLOBAL
+    int		*value_checked)		// value was checked to be save, no
+					// need to set P_INSECURE
 {
     char_u	*errmsg = NULL;
     char_u	*s, *p;
@@ -6134,10 +6142,9 @@ did_set_string_option(
 	errmsg = e_secure;
     }
 
-    /* Check for a "normal" directory or file name in some options.  Disallow a
-     * path separator (slash and/or backslash), wildcards and characters that
-     * are often illegal in a file name. Be more permissive if "secure" is off.
-     */
+    // Check for a "normal" directory or file name in some options.  Disallow a
+    // path separator (slash and/or backslash), wildcards and characters that
+    // are often illegal in a file name. Be more permissive if "secure" is off.
     else if (((options[opt_idx].flags & P_NFNAME)
 		    && vim_strpbrk(*varp, (char_u *)(secure
 			    ? "/\\*?[|;&<>\r\n" : "/\\*?[<>\r\n")) != NULL)
@@ -6524,9 +6531,23 @@ did_set_string_option(
 	if (!valid_filetype(*varp))
 	    errmsg = e_invarg;
 	else
-	    /* load or unload key mapping tables */
+	{
+	    int	    secure_save = secure;
+
+	    // Reset the secure flag, since the value of 'keymap' has
+	    // been checked to be safe.
+	    secure = 0;
+
+	    // load or unload key mapping tables
 	    errmsg = keymap_init();
 
+	    secure = secure_save;
+
+	    // Since we check the value, there is no need to set P_INSECURE,
+	    // even when the value comes from a modeline.
+	    *value_checked = TRUE;
+	}
+
 	if (errmsg == NULL)
 	{
 	    if (*curbuf->b_p_keymap != NUL)
@@ -7523,7 +7544,13 @@ did_set_string_option(
 	if (!valid_filetype(*varp))
 	    errmsg = e_invarg;
 	else
+	{
 	    value_changed = STRCMP(oldval, *varp) != 0;
+
+	    // Since we check the value, there is no need to set P_INSECURE,
+	    // even when the value comes from a modeline.
+	    *value_checked = TRUE;
+	}
     }
 
 #ifdef FEAT_SYN_HL
@@ -7532,7 +7559,13 @@ did_set_string_option(
 	if (!valid_filetype(*varp))
 	    errmsg = e_invarg;
 	else
+	{
 	    value_changed = STRCMP(oldval, *varp) != 0;
+
+	    // Since we check the value, there is no need to set P_INSECURE,
+	    // even when the value comes from a modeline.
+	    *value_checked = TRUE;
+	}
     }
 #endif
 
@@ -7752,7 +7785,12 @@ did_set_string_option(
 	     * already set to this value. */
 	    if (!(opt_flags & OPT_MODELINE) || value_changed)
 	    {
-		static int ft_recursive = 0;
+		static int  ft_recursive = 0;
+		int	    secure_save = secure;
+
+		// Reset the secure flag, since the value of 'filetype' has
+		// been checked to be safe.
+		secure = 0;
 
 		++ft_recursive;
 		did_filetype = TRUE;
@@ -7764,6 +7802,8 @@ did_set_string_option(
 		/* Just in case the old "curbuf" is now invalid. */
 		if (varp != &(curbuf->b_p_ft))
 		    varp = NULL;
+
+		secure = secure_save;
 	    }
 	}
 #ifdef FEAT_SPELL

src/testdir/test_modeline.vim

@@ -6,7 +6,83 @@ func Test_modeline_invalid()
   let modeline = &modeline
   set modeline
   call assert_fails('split Xmodeline', 'E518:')
+
   let &modeline = modeline
   bwipe!
   call delete('Xmodeline')
 endfunc
+
+func Test_modeline_filetype()
+  call writefile(['vim: set ft=c :', 'nothing'], 'Xmodeline_filetype')
+  let modeline = &modeline
+  set modeline
+  filetype plugin on
+  split Xmodeline_filetype
+  call assert_equal("c", &filetype)
+  call assert_equal(1, b:did_ftplugin)
+  call assert_equal("ccomplete#Complete", &ofu)
+
+  bwipe!
+  call delete('Xmodeline_filetype')
+  let &modeline = modeline
+  filetype plugin off
+endfunc
+
+func Test_modeline_syntax()
+  call writefile(['vim: set syn=c :', 'nothing'], 'Xmodeline_syntax')
+  let modeline = &modeline
+  set modeline
+  syntax enable
+  split Xmodeline_syntax
+  call assert_equal("c", &syntax)
+  call assert_equal("c", b:current_syntax)
+
+  bwipe!
+  call delete('Xmodeline_syntax')
+  let &modeline = modeline
+  syntax off
+endfunc
+
+func Test_modeline_keymap()
+  call writefile(['vim: set keymap=greek :', 'nothing'], 'Xmodeline_keymap')
+  let modeline = &modeline
+  set modeline
+  split Xmodeline_keymap
+  call assert_equal("greek", &keymap)
+  call assert_match('greek\|grk', b:keymap_name)
+
+  bwipe!
+  call delete('Xmodeline_keymap')
+  let &modeline = modeline
+  set keymap= iminsert=0 imsearch=-1
+endfunc
+
+func s:modeline_fails(what, text)
+  let fname = "Xmodeline_fails_" . a:what
+  call writefile(['vim: set ' . a:text . ' :', 'nothing'], fname)
+  let modeline = &modeline
+  set modeline
+  filetype plugin on
+  syntax enable
+  call assert_fails('split ' . fname, 'E474:')
+  call assert_equal("", &filetype)
+  call assert_equal("", &syntax)
+
+  bwipe!
+  call delete(fname)
+  let &modeline = modeline
+  filetype plugin off
+  syntax off
+endfunc
+
+func Test_modeline_filetype_fails()
+  call s:modeline_fails('filetype', 'ft=evil$CMD')
+endfunc
+
+func Test_modeline_syntax_fails()
+  call s:modeline_fails('syntax', 'syn=evil$CMD')
+endfunc
+
+func Test_modeline_keymap_fails()
+  call s:modeline_fails('keymap', 'keymap=evil$CMD')
+endfunc

src/version.c

@@ -792,6 +792,8 @@ static char *(features[]) =
 
 static int included_patches[] =
 {   /* Add new patch number below this line */
+/**/
+    544,
 /**/
     543,
 /**/