[RFC PATCH] Fix segmentation fault of listing kprocess.create

[RFC PATCH] Fix segmentation fault of listing kprocess.create

[Wenji Huang]

Hi,

I got the Segmentation fault when executing stap -L kprocess.create for latest
source. The same issue happens on FC11 32bits and RHEL5U2 (64bits 2.6.32 kernel),
with elfutils 0.141-0.143. But  'probe kprocess.create{print(task) print(new_pid) print($$parms)}'
works fine on those machines.

The error is from systemtap_session::print_token, invalid pointer 'tok->location.file'
I am not sure what can cause that.  There is one workaround for this error. Moreover,
I think it's necessary to do some sanity checking when referring to pointer.

Example:
$ stap -L kprocess.create
semantic error: probe_1856 with unresolved type: junk '' at unknown file:0:0
semantic error: probe_1856 with unresolved type: unknown token '' at unknown file:0:0
kprocess.create new_pid:long task:long $cgroup_callbacks_done:int $child_tidptr:int* $clone_flags:long unsigned int $p:struct task_struct* $pid:struct pid* $regs:struct pt_regs* $return:struct task_struct* $retval:int $stack_size:long unsigned int $stack_start:long unsigned int $trace:int

diff --git a/elaborate.cxx b/elaborate.cxx
index 626db28..32fb47f 100644
--- a/elaborate.cxx
+++ b/elaborate.cxx
@@ -1556,9 +1556,11 @@ systemtap_session::print_token (ostream& o, const token* tok)
       tmpo << *tok;
       string ts = tmpo.str();
       // search & replace the file name with nothing
-      size_t idx = ts.find (tok->location.file->name);
-      if (idx != string::npos)
-          ts.replace (idx, tok->location.file->name.size(), "");
+      if (tok->location.file) {
+         size_t idx = ts.find (tok->location.file->name);
+         if (idx != string::npos)
+            ts.replace (idx, tok->location.file->name.size(), "");
+      }

       o << ts;
     }
diff --git a/parse.cxx b/parse.cxx
index cfefa12..5b9005f 100644
--- a/parse.cxx
+++ b/parse.cxx
@@ -91,8 +91,11 @@ tt2str(token_type tt)
 ostream&
 operator << (ostream& o, const source_loc& loc)
 {
-  o << loc.file->name << ":"
-    << loc.line << ":"
+  if (loc.file)
+     o << loc.file->name << ":";
+  else
+     o << "unknown file" << ":";
+  o << loc.line << ":"
     << loc.column;

   return o;
diff --git a/parse.h b/parse.h
index 5587586..2b21f65 100644
--- a/parse.h
+++ b/parse.h
@@ -26,6 +26,8 @@ struct source_loc
   stapfile* file;
   unsigned line;
   unsigned column;
+  source_loc():
+    file(0),line(0),column(0) {}
 };

 std::ostream& operator << (std::ostream& o, const source_loc& loc);

RE: [RFC PATCH] Fix segmentation fault of listing kprocess.create

[Wenji Huang]

> Hi,
> 
> I got the Segmentation fault when executing stap -L 
> kprocess.create for latest
> source. The same issue happens on FC11 32bits and RHEL5U2 
> (64bits 2.6.32 kernel),
> with elfutils 0.141-0.143. But  'probe 
> kprocess.create{print(task) print(new_pid) print($$parms)}'
> works fine on those machines.
> 
Please ignore the previous patch, the root cause is the following section
tapsets.cxx: dwarf_derived_probe::saveargs

         /* trick from visit_target_symbol_context */
         target_symbol *tsym = new target_symbol;
         token *t = new token;
         tsym->tok = t;
         tsym->base_name = "$";
         tsym->base_name += arg_name;

The empty token is created, I will fix it soon. 
Also any suggestion is welcome!

Regards,
Wenji

RE: [RFC PATCH] Fix segmentation fault of listing kprocess.create

[Wenji Huang]

Hi,

Sorry for a little annoying thread. ^-^
> Please ignore the previous patch, the root cause is the 
> following section
> tapsets.cxx: dwarf_derived_probe::saveargs
> 
>          /* trick from visit_target_symbol_context */
>          target_symbol *tsym = new target_symbol;
>          token *t = new token;
>          tsym->tok = t;
>          tsym->base_name = "$";
>          tsym->base_name += arg_name;
> 
> The empty token is created, I will fix it soon. 
> Also any suggestion is welcome!
> 
Take a deep look. Seems we will try to access the variables in
return probe. It's fully supported or not? Guess not.

If not, the function should return early once has_return is true.
If yes, we will hypothesize accessing the argument. And the
source_loc of t has to be q.base_loc since no real one.

Regards,
Wenji

Re: [RFC PATCH] Fix segmentation fault of listing kprocess.create

[Frank Ch. Eigler]

"Wenji Huang" <wenji.huang@oracle.com> writes:

>> Please ignore the previous patch, the root cause is the 
>> following section tapsets.cxx: dwarf_derived_probe::saveargs

Indeed.

>>          /* trick from visit_target_symbol_context */
>>          target_symbol *tsym = new target_symbol;
>>          token *t = new token;
>>          tsym->tok = t;
>>          tsym->base_name = "$";
>>          tsym->base_name += arg_name;

Right, such an empty token should not exist.  (We may be able to
remove this default constructor and force clients to fill in the
fields immediately.)

When synthesizing new parse tree structures, the token pointer
assigned to the new objects usually relates to the original
script-level object that caused the synthesis.  So for example in a
return probe that uses a saved entry-time $var, a whole new synthetic
probe and global variables could all be assigned to the same "$var"
token.

Perhaps the recently introduced saveargs() function should be supplied
with an appropriate token*, for examples q.base_probe->tok.

- FChE

Re: [RFC PATCH] Fix segmentation fault of listing kprocess.create

[Wenji Huang]

fche@redhat.com wrote:
>>>          /* trick from visit_target_symbol_context */
>>>          target_symbol *tsym = new target_symbol;
>>>          token *t = new token;
>>>          tsym->tok = t;
>>>          tsym->base_name = "$";
>>>          tsym->base_name += arg_name;
> 
> Right, such an empty token should not exist.  (We may be able to
> remove this default constructor and force clients to fill in the
> fields immediately.)
> 
> When synthesizing new parse tree structures, the token pointer
> assigned to the new objects usually relates to the original
> script-level object that caused the synthesis.  So for example in a
> return probe that uses a saved entry-time $var, a whole new synthetic
> probe and global variables could all be assigned to the same "$var"
> token.
> 
> Perhaps the recently introduced saveargs() function should be supplied
> with an appropriate token*, for examples q.base_probe->tok.

The empty token is from my patch for 10820, sorry for the
error.  Maybe the following patch can fix that.

Regards,
Wenji

diff --git a/tapsets.cxx b/tapsets.cxx
index d2c3334..17e315e 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -2928,6 +2928,14 @@ dwarf_derived_probe::saveargs(dwarf_query& q, 
Dwarf_Die* scope_die, dwarf_var_ex
          /* trick from visit_target_symbol_context */
          target_symbol *tsym = new target_symbol;
          token *t = new token;
+        /* We hypothesize accessing the argument
+         * The source_loc will be base_loc since no real one  */
+        t->content = "$";
+        t->content += arg_name;
+        t->type = tok_identifier;
+        t->location.file = q.base_loc->tok->location.file;
+        t->location.column = q.base_loc->tok->location.column;
+        t->location.line = q.base_loc->tok->location.line;
          tsym->tok = t;
          tsym->base_name = "$";
          tsym->base_name += arg_name;

Re: [RFC PATCH] Fix segmentation fault of listing kprocess.create

[Frank Ch. Eigler]

Hi -

On Fri, Nov 06, 2009 at 09:06:25AM +0800, Wenji Huang wrote:
> [...]
> The empty token is from my patch for 10820, sorry for the
> error.  [...]

No problem.

> diff --git a/tapsets.cxx b/tapsets.cxx
> index d2c3334..17e315e 100644
> --- a/tapsets.cxx
> +++ b/tapsets.cxx
> @@ -2928,6 +2928,14 @@ dwarf_derived_probe::saveargs(dwarf_query& q, 
> Dwarf_Die* scope_die, dwarf_var_ex
>          /* trick from visit_target_symbol_context */
>          target_symbol *tsym = new target_symbol;
>          token *t = new token;
> +        /* We hypothesize accessing the argument
> +         * The source_loc will be base_loc since no real one  */
> +        t->content = "$";
> +        t->content += arg_name;
> +        t->type = tok_identifier;
> +        t->location.file = q.base_loc->tok->location.file;
> +        t->location.column = q.base_loc->tok->location.column;
> +        t->location.line = q.base_loc->tok->location.line;
>          tsym->tok = t;

You shouldn't need to synthesize this either really.  Just copy the
pointer from q.base_loc->tok, if we don't have a vardecl or expression
handy.

- FChE

Re: [RFC PATCH] Fix segmentation fault of listing kprocess.create

[Josh Stone]

On 11/04/2009 11:38 PM, Wenji Huang wrote:
> Hi,
> 
> Sorry for a little annoying thread. ^-^
>> Please ignore the previous patch, the root cause is the 
>> following section
>> tapsets.cxx: dwarf_derived_probe::saveargs
>>
>>          /* trick from visit_target_symbol_context */
>>          target_symbol *tsym = new target_symbol;
>>          token *t = new token;
>>          tsym->tok = t;
>>          tsym->base_name = "$";
>>          tsym->base_name += arg_name;
>>
>> The empty token is created, I will fix it soon. 
>> Also any suggestion is welcome!
>>
> Take a deep look. Seems we will try to access the variables in
> return probe. It's fully supported or not? Guess not.
> 
> If not, the function should return early once has_return is true.
> If yes, we will hypothesize accessing the argument. And the
> source_loc of t has to be q.base_loc since no real one.

You were on to the real problem here, and then you and Frank kept
talking about token pointers...  Yes, we do support reading variables in
a return probe, but that's playing funny games with saveargs.

The issue is that when a $var is accessed in a return probe, the
dwarf_var_expanding_visitor will create a matching .call which does the
real $var access.  The .call will get resolved later, so within this
saveargs it will always appear successful.

I committed d87623a which momentarily suspends the has_return status, so
that dwarf_var_expanding_visitor won't try to do anything funny.  This
appears to work for me, so let me know if it still gives you trouble.

Josh