[FYI/pushed] (Ada) ada-tasks reading uninitialized inferior memory

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[FYI/pushed] (Ada) ada-tasks reading uninitialized inferior memory

Joel Brobecker
Hello,

I just pushed the following two patches to master, to fix a latent
issue where ada-tasks.c's read_atcb sometimes read uninitialized
memory in the inferior. Up to recently, this wouldn't cause any
actual problem, but a recent change to AdaCore's compiler revealed
the weakness.

  * [PATCH 1/2] ada-tasks.c::read_atcb: start from a cleared
  * [PATCH 2/2] (Ada/tasking) fix array or string index out of range

Tested on x86_64-linux and pushed to master.

Thank you,
--
Joel
Reply | Threaded
Open this post in threaded view
|

[FYI/pushed] ada-tasks.c::read_atcb: start from a cleared ada_task_info result

Joel Brobecker
The purpose of this patch is not to fix a bug per se, but rather
to robustify this function to make sure it never returns a struct
ada_task_info where some of the fields are left uninitialized.
Reading the current implementation, it attempts to methodically
set them all one by one: but it's not excluded that a future
change might miss something. A memset is cheap and make sure that
this function returns repeatable results.

This in turns allows us to remove some assignments which have become
redundant.

gdb/ChangeLog:

        * ada-tasks.c (read_atcb): Clear task_info before computing
        the value of each of its fields.
---
 gdb/ChangeLog   |  5 +++++
 gdb/ada-tasks.c | 16 ++++++----------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f7d1de5..c94321b 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-11-07  Joel Brobecker  <[hidden email]>
+
+ * ada-tasks.c (read_atcb): Clear task_info before computing
+ the value of each of its fields.
+
 2018-11-07  Andrew Burgess  <[hidden email]>
 
  * dwarf2read.c (dwarf2_init_integer_type): Check for name being
diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index 566eae5..5b97e93 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -611,6 +611,10 @@ read_atcb (CORE_ADDR task_id, struct ada_task_info *task_info)
   const struct ada_tasks_pspace_data *pspace_data
     = get_ada_tasks_pspace_data (current_program_space);
 
+  /* Clear the whole structure to start with, so that everything
+     is always initialized the same.  */
+  memset (task_info, 0, sizeof (struct ada_task_info));
+
   if (!pspace_data->initialized_p)
     {
       const char *err_msg = ada_get_tcb_types_info ();
@@ -704,9 +708,6 @@ read_atcb (CORE_ADDR task_id, struct ada_task_info *task_info)
     task_info->parent =
       value_as_address (value_field (common_value,
      pspace_data->atcb_fieldno.parent));
-  else
-    task_info->parent = 0;
-  
 
   /* If the ATCB contains some information about entry calls, then
      compute the "called_task" as well.  Otherwise, zero.  */
@@ -732,15 +733,10 @@ read_atcb (CORE_ADDR task_id, struct ada_task_info *task_info)
         value_as_address (value_field (entry_calls_value_element,
                                        called_task_fieldno));
     }
-  else
-    {
-      task_info->called_task = 0;
-    }
 
-  /* If the ATCB cotnains some information about RV callers,
-     then compute the "caller_task".  Otherwise, zero.  */
+  /* If the ATCB cotnains some information about RV callers, then
+     compute the "caller_task".  Otherwise, leave it as zero.  */
 
-  task_info->caller_task = 0;
   if (pspace_data->atcb_fieldno.call >= 0)
     {
       /* Get the ID of the caller task from Common_ATCB.Call.all.Self.
--
2.1.4

Reply | Threaded
Open this post in threaded view
|

[FYI/pushed] (Ada/tasking) fix array or string index out of range warning

Joel Brobecker
In reply to this post by Joel Brobecker
A recent change in the compiler highlighted a small weakness in
the function reading the contents of the Ada Task Control Block
(ATCB -- the data that allows us to inspect Ada tasks). As a result,
anytime we read it, we started getting some warnings. For instance,
using the gdb.ada/tasks.exp testcase...

        $ gnatmake -g foo.adb
        $ gdb foo
        (gdb) b foo.adb:60
        Breakpoint 1 at 0x403e07: file foo.adb, line 60.
        (gdb) run
        [...]
        Thread 1 "foo" hit Breakpoint 1, foo () at foo.adb:60
        60         for J in Task_List'Range loop  -- STOP_HERE

... we can see that the "info tasks" command produces some warnings,
followed by the correct output.

        (gdb) info tasks
  !! ->  warning: array or string index out of range
  !! ->  warning: array or string index out of range
  !! ->  warning: array or string index out of range
  !! ->  warning: array or string index out of range
           ID       TID P-ID Pri State                  Name
        *   1    654050       48 Runnable               main_task
            2    654ef0    1  48 Accept or Select Term  task_list(1)
            3    658680    1  48 Accept or Select Term  task_list(2)
            4    65be10    1  48 Accept or Select Term  task_list(3)

The problem comes from the fact that read_atcb, the function responsible
for loading the contents of the ATCB, blindly tries to read some data
which is only relevant when a task is waiting for another task on
an entry call. A comment in that code's section gives a hint as to
how the information is meant to be decoded:

      /* Let My_ATCB be the Ada task control block of a task calling the
         entry of another task; then the Task_Id of the called task is
         in My_ATCB.Entry_Calls (My_ATCB.ATC_Nesting_Level).Called_Task.  */

What the comment shows is that, to get the Id of the task being called,
one has to go through the entry calls field, which is an array pointer.
Up to now, we were lucky that, for tasks that are _not_ waiting on an
entry call, its ATCB atc_nesting_level used to be set to 1, and so
we were able to silently read some irrelevant data. But a recent change
now causes this field to be zero instead, and this triggers the warning,
since we are now trying to read outside of the array's range (arrays
in Ada often start at index 1, as is the case here).

We avoid this issue by simply only reading that data when the data
is actually known to be relevant (state == Entry_Caller_Sleep).

This, in turn, allows us to simplify a bit the use of the task_info->state
field, where we no longer need to check task the task has a state equal
to Entry_Caller_Sleep before using this field. Indeed, with this new
approach, we now know that, unless task_info->state == Entry_Caller_Sleep,
the state is now guaranteed to be zero. In other words, we no longer set
task_info->called_task to some random value, forcing to check the task's
state first as a way to verify that the data is not random.

gdb/ChangeLog:

        * ada-lang.c (read_atcb): Only set task_info->called_task if
        task_info->state == Entry_Caller_Sleep.
        (print_ada_task_info): Do not check task_info->state before
        checking task_info->called_task.
        (info_task): Likewise.
---
 gdb/ChangeLog   |  8 ++++++++
 gdb/ada-tasks.c | 12 ++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index c94321b..9c277f5 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2018-11-07  Joel Brobecker  <[hidden email]>
 
+ * ada-lang.c (read_atcb): Only set task_info->called_task if
+ task_info->state == Entry_Caller_Sleep.
+ (print_ada_task_info): Do not check task_info->state before
+ checking task_info->called_task.
+ (info_task): Likewise.
+
+2018-11-07  Joel Brobecker  <[hidden email]>
+
  * ada-tasks.c (read_atcb): Clear task_info before computing
  the value of each of its fields.
 
diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index 5b97e93..71af616 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -709,10 +709,11 @@ read_atcb (CORE_ADDR task_id, struct ada_task_info *task_info)
       value_as_address (value_field (common_value,
      pspace_data->atcb_fieldno.parent));
 
-  /* If the ATCB contains some information about entry calls, then
-     compute the "called_task" as well.  Otherwise, zero.  */
+  /* If the task is in an entry call waiting for another task,
+     then determine which task it is.  */
 
-  if (pspace_data->atcb_fieldno.atc_nesting_level > 0
+  if (task_info->state == Entry_Caller_Sleep
+      && pspace_data->atcb_fieldno.atc_nesting_level > 0
       && pspace_data->atcb_fieldno.entry_calls > 0)
     {
       /* Let My_ATCB be the Ada task control block of a task calling the
@@ -1126,8 +1127,7 @@ print_ada_task_info (struct ui_out *uiout,
   _("Accepting RV with %-4d"),
   get_task_number_from_id (task_info->caller_task,
    inf));
-      else if (task_info->state == Entry_Caller_Sleep
-       && task_info->called_task)
+      else if (task_info->called_task)
  uiout->field_fmt ("state",
   _("Waiting on RV with %-3d"),
   get_task_number_from_id (task_info->called_task,
@@ -1213,7 +1213,7 @@ info_task (struct ui_out *uiout, const char *taskno_str, struct inferior *inf)
         printf_filtered (_("State: Accepting rendezvous with %d"),
                          target_taskno);
       }
-    else if (task_info->state == Entry_Caller_Sleep && task_info->called_task)
+    else if (task_info->called_task)
       {
         target_taskno = get_task_number_from_id (task_info->called_task, inf);
         printf_filtered (_("State: Waiting on task %d's entry"),
--
2.1.4