Re: [libunwind] bug (and suggested fix): desc_alias() and zero-size regions

[David Mosberger]

>>>>> On Mon, 7 Mar 2005 17:01:14 -0600 (CST), Todd L Miller <address@hidden> 
>>>>> said:

  Todd> On line 759 of the 0.98.2 release, desc_alias() calculates 'when':
  Todd> when = MIN(sr->when_target, rlen - 1);

  Todd> and then uses 'when' to calculate 'new_ip':

  Todd> new_ip = op->val + ((when / 3) * 16 + (when % 3));

  Todd> When you pass in a zero-size region, new_ip is one less than
  Todd> op-> val, e.g., 0x4...0cf, instead of 0x4...0d0.  This breaks
  Todd> create_state_record_for()'s calculation of the new region's
  Todd> when_target.  This can cause incorrect stackwalks: in
  Todd> particular, I triggered a case in which the location of the
  Todd> preserved RP changed between the corrected when_target and the
  Todd> one calculated by calculate_state_record_for().

  Todd> My work-around is simply to set new_ip to op->val when 'when'
  Todd> is -1 in desc_alias().  It works, but I'm not sure that it's
  Todd> the right thing to do.  I'd also guess that the other two uses
  Todd> of MIN( , rlen - 1 ) in the function need to be fixed, though
  Todd> I haven't seen any problems.

OK, this is one of those "simple" problems that are hard to think
about.  I think the correct fix is to use "rlen" instead of "rlen -
1", as shown in the patch below.

The rationale for this is as follows: for the ALIAS directive, RLEN
specifies the number if instructions for which the region behaves like
the aliased code.  For example, if RLEN=1 then the code follows the
aliased region for 1 instruction and the valid WHEN values should be 0
and 1.  However, with the unpatched code, we would have allowed for -1
and 0, which is wrong.

Can you verify that this patch works as expected for you?

Thanks and sorry for the slow response,

        --david

===== src/ia64/Gparser.c 1.39 vs edited =====
--- 1.39/src/ia64/Gparser.c     2005-04-07 12:19:38 -07:00
+++ edited/src/ia64/Gparser.c   2005-04-07 15:48:15 -07:00
@@ -747,7 +747,8 @@
 /* An alias directive inside a region of length RLEN is interpreted to
    mean that the region behaves exactly like the first RLEN
    instructions at the aliased IP.  RLEN=0 implies that the current
-   state matches exactly that of the aliased IP.  */
+   state matches exactly that of before the instruction at the aliased
+   IP is executed.  */
 
 static int
 desc_alias (unw_dyn_op_t *op, struct cursor *c, struct ia64_state_record *sr)
@@ -756,7 +757,7 @@
   int i, ret, when, rlen = sr->region_len;
   unw_word_t new_ip;
 
-  when = MIN(sr->when_target, rlen - 1);
+  when = MIN (sr->when_target, rlen);
   new_ip = op->val + ((when / 3) * 16 + (when % 3));
 
   if ((ret = ia64_fetch_proc_info (c, new_ip, 1)) < 0)
@@ -772,13 +773,13 @@
   sr->region_start = orig_sr.region_start;
   sr->region_len = orig_sr.region_len;
   if (sr->when_sp_restored != IA64_WHEN_NEVER)
-    sr->when_sp_restored = op->when + MIN (orig_sr.when_sp_restored, rlen - 1);
+    sr->when_sp_restored = op->when + MIN (orig_sr.when_sp_restored, rlen);
   sr->epilogue_count = orig_sr.epilogue_count;
   sr->when_target = orig_sr.when_target;
 
   for (i = 0; i < IA64_NUM_PREGS; ++i)
     if (sr->curr.reg[i].when != IA64_WHEN_NEVER)
-      sr->curr.reg[i].when = op->when + MIN (sr->curr.reg[i].when, rlen - 1);
+      sr->curr.reg[i].when = op->when + MIN (sr->curr.reg[i].when, rlen);
 
   ia64_free_state_record (sr);
   sr->labeled_states = orig_sr.labeled_states;