Change in binutils-gdb[master]: [RFC][gdb] Make script-extension strict reject unknown extensions

Change in binutils-gdb[master]: [RFC][gdb] Make script-extension strict reject unknown extensions

[Christian Biesinger (Code Review)]

Christian Biesinger has posted comments on this change. ( https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40 )

Change subject: [RFC][gdb] Make script-extension strict reject unknown extensions
......................................................................


Patch Set 1:

A couple questions about this:
- If specified in -ix or a system gdbinit file, does it prevent reading ~/.gdbinit?
- Does it prevent reading a file in --command if it doesn't have the extension?


-- 
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings

Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ia357a4b0b1042808401578266da80919035ad576
Gerrit-Change-Number: 40
Gerrit-PatchSet: 1
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Christian Biesinger <cbiesinger@google.com>
Gerrit-Comment-Date: Mon, 14 Oct 2019 16:08:20 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in binutils-gdb[master]: [RFC][gdb] Make script-extension strict reject unknown extensions

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change. ( https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40 )

Change subject: [RFC][gdb] Make script-extension strict reject unknown extensions
......................................................................


Patch Set 1:

> Patch Set 1:
> 
> A couple questions about this:
> - If specified in -ix or a system gdbinit file, does it prevent reading ~/.gdbinit?

I marked ~/.gdbinit execution using:
...
$ cat ~/.gdbinit
python
print ("HERE")
...

And using this command I can see iex is executed after ~/.gdbinit:
...
$ gdb -q -batch -iex "echo bla1\n" -iex "set script-extension strict" -iex "echo bla2\n"
HERE
bla1
bla2
...

But for the system gdbinit, it's yes.

> - Does it prevent reading a file in --command if it doesn't have the extension?

Yes:
...
$ gdb -batch -q -ex "set script-extension strict" --command=bla.bla
Scripting in the "<UNKNOWN>" language is not supported in this copy of GDB.
....


-- 
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings

Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ia357a4b0b1042808401578266da80919035ad576
Gerrit-Change-Number: 40
Gerrit-PatchSet: 1
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Christian Biesinger <cbiesinger@google.com>
Gerrit-Comment-Date: Mon, 14 Oct 2019 17:41:45 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] [RFC][gdb] Make script-extension strict reject unknown extensions

[Christian Biesinger (Code Review)]

Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40
......................................................................


Patch Set 1:

> Patch Set 1:
> 
> > Patch Set 1:
> > 
> > A couple questions about this:
> > - If specified in -ix or a system gdbinit file, does it prevent reading ~/.gdbinit?
[...] 
> But for the system gdbinit, it's yes.
> 
> > - Does it prevent reading a file in --command if it doesn't have the extension?
> 
> Yes:
[...]

Hm, personally I would find that behavior surprising, especially for the gdbinit file. But I have no decision power here...

[review v2] [RFC][gdb] Make script-extension strict reject unknown extensions

[Tom de Vries (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40
......................................................................

[RFC][gdb] Make script-extension strict reject unknown extensions

At
https://sourceware.org/gdb/onlinedocs/gdb.html#index-set-script_002dextension
we have:
...
Files with an unrecognized filename extension are always treated as a GDB
Command Files.
...

So, we have this behaviour:
...
$ gdb -q -batch a.out \
  -ex "set trace-commands on" \
  -ex "set script-extension strict" \
  -ex "show script-extension" \
  -ex "source bla.bla"
+set script-extension strict
+show script-extension
Script filename extension recognition is "strict".
+source bla.bla
+start
Temporary breakpoint 1 at 0x40050b: file /home/vries/hello.c, line 6.

Temporary breakpoint 1, main () at /home/vries/hello.c:6
6         printf ("hello\n");
+bt
...

Change this behaviour to allow only recognized extensions when using strict:
...
$ gdb -q -batch a.out \
  -ex "set trace-commands on" \
  -ex "set script-extension strict" \
  -ex "show script-extension" \
  -ex "source bla.bla"
+set script-extension strict
+show script-extension
Script filename extension recognition is "strict".
+source bla.bla
Scripting in the "<UNKNOWN>" language is not supported in this copy of GDB.
...

This does not change the default behaviour of gdb, which uses the
'script-extension soft' setting.

Setting script-extension to force doesn't inhibit reading:
- system gdbinit (/etc/gdbinit)
- home gdbinit (~/.gdbinit)
- local gdbinit (./.gdbinit)

Tested on x86_64-linux.

No docs update yet.

gdb/ChangeLog:

2019-10-22  Tom de Vries  <tdevries@suse.de>

	* cli/cli-cmds.c (source_script_from_stream)
	(source_script_with_search): Add and handle force parameter.
	(source_script_force): New function.
	* cli/cli-cmds.h (source_script_force): Declare.
	* extension.c (extension_language_none): New var.
	(get_ext_lang_of_file): Handle extension_language_gdb.suffix.
	* extension.h (extension_language_none): Declare.
	* main.c (captured_main_1): Use source_script_force for gdbinit files.

gdb/testsuite/ChangeLog:

2019-10-22  Tom de Vries  <tdevries@suse.de>

	* gdb.base/source-execution.exp: Test rejection of gdb.bla when
	script-extension is set to strict.
	* gdb.base/gdb.bla: New empty command file.

Change-Id: Ia357a4b0b1042808401578266da80919035ad576
---
M gdb/cli/cli-cmds.c
M gdb/cli/cli-cmds.h
M gdb/extension.c
M gdb/extension.h
M gdb/main.c
A gdb/testsuite/gdb.base/gdb.bla
M gdb/testsuite/gdb.base/source-execution.exp
7 files changed, 43 insertions(+), 6 deletions(-)



diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index a39ea22..24c0bcb 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -630,7 +630,7 @@
 
 static void
 source_script_from_stream (FILE *stream, const char *file,
-			   const char *file_to_open)
+			   const char *file_to_open, bool force)
 {
   if (script_ext_mode != script_ext_off)
     {
@@ -656,6 +656,10 @@
 	  else
 	    throw_ext_lang_unsupported (extlang);
 	}
+      else if (!force && script_ext_mode == script_ext_strict)
+	{
+	  throw_ext_lang_unsupported (&extension_language_none);
+	}
     }
 
   script_from_file (stream, file);
@@ -667,7 +671,8 @@
    search for it in the source search path.  */
 
 static void
-source_script_with_search (const char *file, int from_tty, int search_path)
+source_script_with_search (const char *file, int from_tty, int search_path,
+			   bool force = false)
 {
 
   if (file == NULL || *file == 0)
@@ -695,7 +700,8 @@
      this if we (may have) used search_path, as printing the full path in
      errors for the non-search case can be more noise than signal.  */
   source_script_from_stream (opened->stream.get (), file,
-			     search_path ? opened->full_path.get () : file);
+			     search_path ? opened->full_path.get () : file,
+			     force);
 }
 
 /* Wrapper around source_script_with_search to export it to main.c
@@ -707,6 +713,12 @@
   source_script_with_search (file, from_tty, 0);
 }
 
+void
+source_script_force (const char *file, int from_tty)
+{
+  source_script_with_search (file, from_tty, 0, true);
+}
+
 static void
 source_command (const char *args, int from_tty)
 {
diff --git a/gdb/cli/cli-cmds.h b/gdb/cli/cli-cmds.h
index 94ae814..16f6147 100644
--- a/gdb/cli/cli-cmds.h
+++ b/gdb/cli/cli-cmds.h
@@ -120,6 +120,7 @@
 extern void quit_command (const char *, int);
 
 extern void source_script (const char *, int);
+extern void source_script_force (const char *, int);
 
 /* Exported to objfiles.c.  */
 
diff --git a/gdb/extension.c b/gdb/extension.c
index 8637bc5..8a7bea5 100644
--- a/gdb/extension.c
+++ b/gdb/extension.c
@@ -66,6 +66,18 @@
   auto_load_gdb_scripts_enabled
 };
 
+const struct extension_language_defn extension_language_none =
+{
+  EXT_LANG_NONE,
+  "<unknown>",
+  "<UNKNOWN>",
+  NULL,
+  NULL,
+  invalid_control,
+  NULL,
+  NULL
+};
+
 const struct extension_language_defn extension_language_gdb =
 {
   EXT_LANG_GDB,
@@ -154,6 +166,9 @@
   int i;
   const struct extension_language_defn *extlang;
 
+  if (has_extension (file, extension_language_gdb.suffix))
+    return &extension_language_gdb;
+
   ALL_EXTENSION_LANGUAGES (i, extlang)
     {
       if (has_extension (file, extlang->suffix))
diff --git a/gdb/extension.h b/gdb/extension.h
index fc8e3e2..a59d15a 100644
--- a/gdb/extension.h
+++ b/gdb/extension.h
@@ -230,6 +230,8 @@
 /* The interface for gdb's own extension(/scripting) language.  */
 extern const struct extension_language_defn extension_language_gdb;
 
+extern const struct extension_language_defn extension_language_none;
+
 extern const struct extension_language_defn *get_ext_lang_defn
   (enum extension_language lang);
 
diff --git a/gdb/main.c b/gdb/main.c
index a77d6ec..c56005a 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -993,7 +993,8 @@
      processed; it sets global parameters, which are independent of
      what file you are debugging or what directory you are in.  */
   if (!system_gdbinit.empty () && !inhibit_gdbinit)
-    ret = catch_command_errors (source_script, system_gdbinit.c_str (), 0);
+    ret = catch_command_errors (source_script_force, system_gdbinit.c_str (),
+				0);
 
   /* Read and execute $HOME/.gdbinit file, if it exists.  This is done
      *before* all the command line arguments are processed; it sets
@@ -1001,7 +1002,7 @@
      debugging or what directory you are in.  */
 
   if (!home_gdbinit.empty () && !inhibit_gdbinit && !inhibit_home_gdbinit)
-    ret = catch_command_errors (source_script, home_gdbinit.c_str (), 0);
+    ret = catch_command_errors (source_script_force, home_gdbinit.c_str (), 0);
 
   /* Process '-ix' and '-iex' options early.  */
   for (i = 0; i < cmdarg_vec.size (); i++)
@@ -1117,7 +1118,8 @@
 	{
 	  auto_load_local_gdbinit_loaded = 1;
 
-	  ret = catch_command_errors (source_script, local_gdbinit.c_str (), 0);
+	  ret = catch_command_errors (source_script_force,
+				      local_gdbinit.c_str (), 0);
 	}
     }
 
diff --git a/gdb/testsuite/gdb.base/gdb.bla b/gdb/testsuite/gdb.base/gdb.bla
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb.bla
diff --git a/gdb/testsuite/gdb.base/source-execution.exp b/gdb/testsuite/gdb.base/source-execution.exp
index 25e7e37..4fa76b3 100644
--- a/gdb/testsuite/gdb.base/source-execution.exp
+++ b/gdb/testsuite/gdb.base/source-execution.exp
@@ -31,3 +31,8 @@
 gdb_test "source ${srcdir}/${subdir}/source-execution.gdb" \
     "func2.*func3.*" \
     "source source-execution.gdb"
+
+gdb_test_no_output "set script-extension strict"
+
+gdb_test "source gdb.bla" \
+    "Scripting in the \"<UNKNOWN>\" language is not supported in this copy of GDB."

[review] [RFC][gdb] Make script-extension strict reject unknown extensions

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40
......................................................................


Patch Set 1:

> Patch Set 1:
> 
> > Patch Set 1:
> > 
> > > Patch Set 1:
> > > 
> > > A couple questions about this:
> > > - If specified in -ix or a system gdbinit file, does it prevent reading ~/.gdbinit?
> [...] 
> > But for the system gdbinit, it's yes.
> > 
> > > - Does it prevent reading a file in --command if it doesn't have the extension?
> > 
> > Yes:
> [...]
> 
> Hm, personally I would find that behavior surprising, especially for the gdbinit file. But I have no decision power here...

Fixed in patch set 2.

[review v2] [RFC][gdb] Make script-extension strict reject unknown extensions

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40
......................................................................


Patch Set 2: Code-Review-1

(2 comments)

Thanks for the patch.

This idea seems reasonable enough to me.

I'm going to mark this "-1" since it needs documentation still (and probably NEWS).
This lets it show up as reviewed in gerrit, but shouldn't be taken to mean some
sort of rejection.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40/2//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40/2//COMMIT_MSG@53 
PS2, Line 53: Setting script-extension to force doesn't inhibit reading:
I guess s/force/strict/ here?


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40/2/gdb/cli/cli-cmds.c 
File gdb/cli/cli-cmds.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40/2/gdb/cli/cli-cmds.c@624 
PS2, Line 624: script FILE, which has already been opened as STREAM.
             :    FILE_TO_OPEN is the form of FILE to use if one needs to open the file.
             :    This is provided as FILE may have been found via the source search path.
             :    An important thing to note here is that FILE may be a symlink to a file
             :    with a different or non-existing suffix, and thus one cannot infer the
             :    extension language from FILE_TO_OPEN.  */
             : 
             : static void
             : source_script_from_stream (FILE *stream, const char *file,
             : 		
The comment should be updated to document FORCE.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ia357a4b0b1042808401578266da80919035ad576
Gerrit-Change-Number: 40
Gerrit-PatchSet: 2
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Christian Biesinger <cbiesinger@google.com>
Gerrit-Comment-Date: Fri, 25 Oct 2019 17:32:09 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v2] [RFC][gdb] Make script-extension strict reject unknown extensions

[Christian Biesinger (Code Review)]

Christian Biesinger has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40
......................................................................


Patch Set 2:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40/2/gdb/cli/cli-cmds.h 
File gdb/cli/cli-cmds.h:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/40/2/gdb/cli/cli-cmds.h@123 
PS2, Line 123: extern void source_script_force (const char *, int);
This should probably have some documentation on what it forces



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ia357a4b0b1042808401578266da80919035ad576
Gerrit-Change-Number: 40
Gerrit-PatchSet: 2
Gerrit-Owner: Tom de Vries <tdevries@suse.de>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
Gerrit-CC: Christian Biesinger <cbiesinger@google.com>
Gerrit-Comment-Date: Fri, 25 Oct 2019 17:46:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment