duktape/bugs/issue-4df1a3df6fcfc88b7515221dd083243df8747cb0.yaml

--- !ditz.rubyforge.org,2008-03-06/issue 
title: DUK_TVAL_SET_TVAL corruption in FreeBSD + clang
desc: |-
  To reproduce:
  
    * Compile in FreeBSD with clang 3.3:
  
      $ clang -v
      FreeBSD clang version 3.3 (tags/RELEASE_33/final 183502) 20130610
      Target: x86_64-unknown-freebsd10.0
      Thread model: posix
  
    * Additional options: -m32, DUK_PROFILE=101
  
    * Hand tune duk_features.h to enable all log levels 
  
    * The problem persists even when -fstrict-aliasing is NOT given, but
      disappears if -Os is changed to -O0.
  
  Incorrect behavior happens in multiple places (not sure if this is a clang/llvm
  bug or incorrect assumptions in Duktape aliasing related code).
  
  In duk_push_tval(), after DUK_TVAL_SET_TVAL(tv_slot, tv) has been called, the
  memory dump of the two regions are (first byte is from 'tv', second is from
  'tv_slot')::
  
      60 60
      32 32
      81 81
      28 28
      00 00
      00 00
      f5 fd   <==
      ff ff
  
  As can be seen, the second to last byte has been changed from F5 to FD, which
  causes all sorts of problems.  One possible culprit may be the DUK_TVAL_SET_TVAL()
  macro itself, which simply uses *(dst) = *(src) for a duk_double_union - perhaps
  this is non-portable.  The problem persist even when replaced by a memmove() or
  by (dst)->d = (src)->d.
  
  Copying the values byte by byte using the 'uc' part of the union fixes this
  particular problem::
  
      #define DUK_TVAL_SET_TVAL(v,x) do { \
          volatile int _i; \
          for (_i = 0; _i < sizeof(duk_tval); _i++) { \
              ((unsigned char *) (v))[_i] = ((unsigned char *) (x))[_i]; \
          } \
      } while (0)
  
  Note that if you remove "volatile", the fix works in most call sites but not
  all!  This suggests clang/llvm converts the hand-coded byte copy to some
  intrinsic which fails to honor the aliasing rules for char pointers.
  
  Another problem seems to happen in duk_hobject_props.c:realloc_props, at the
  line:
  
      new_e_pv[new_e_used] = DUK_HOBJECT_E_GET_VALUE(obj, i);
  
  The line copies a duk_propvalue to another using a plain assignment.  Debug
  printing the results shows that the values get corrupted in the copy the same
  way as in DUK_TVAL_SET_TVAL.  Some values copy OK (first is source, second is
  target):
  
      00 00
      00 00
      00 00
      00 00
      00 00
      00 00
      f8 f8
      7f 7f
  
      00 00
      00 00
      00 00
      00 00
      00 00
      00 00
      f0 f0
      7f 7f
  
  while others get corrupted again at their 7th byte (little endian memory layout).
  In more concrete terms, 0x08 gets OR'd to the 7th byte.  Considering the double
  as a 64-bit big endian value, 0x0008'0000'0000'0000 gets OR'd to the value:
  
      2c 2c
      cb cb
      ff ff
      ff ff
      00 00
      00 00
      f1 f9   <==
      ff ff
  
      10 10
      49 49
      81 81
      28 28
      00 00
      00 00
      f6 fe   <==
      ff ff
  
  The bit in question is the highest bit of the fractional part.  This can be
  similarly fixed by writing a manual byte-by-byte copy, again with a volatile
  loop counter.  
type: :bugfix
component: duk
release: 
reporter: sva <sami.vaarala@iki.fi>
status: :unstarted
disposition: 
creation_time: 2013-12-10 01:28:45.479389 Z
references: []

id: 4df1a3df6fcfc88b7515221dd083243df8747cb0
log_events: 
- - 2013-12-10 01:28:45.647113 Z
  - sva <sami.vaarala@iki.fi>
  - created
  - ""
- - 2013-12-10 01:30:44.318909 Z
  - sva <sami.vaarala@iki.fi>
  - commented
  - |-
    Below is a diff of the two fixes mentioned in the description, applied to
    the all-in-one source, which together fix the crashes in FreeBSD + clang
    3.3.  The diff is not directly applicable because it is a bit hacky, but
    illustrates the idea of the fix.
    
    Removing the 'volatile' for either of the loop variables breaks the result.
    
    $ diff -u duktape-orig.c duktape-fixed.c 
    --- duktape-orig.c	2013-12-10 04:28:08.000000000 +0200
    +++ duktape-fixed.c	2013-12-10 04:28:15.000000000 +0200
    @@ -5832,7 +5832,12 @@
     #define DUK_TVAL_SET_BUFFER(v,h)            DUK__TVAL_SET_TAGGEDPOINTER((v),(h),DUK_TAG_BUFFER)
     #define DUK_TVAL_SET_POINTER(v,p)           DUK__TVAL_SET_TAGGEDPOINTER((v),(p),DUK_TAG_POINTER)
    
    -#define DUK_TVAL_SET_TVAL(v,x)              do { *(v) = *(x); } while (0)
    +#define DUK_TVAL_SET_TVAL(v,x)              do { \
    +	volatile int _i; \
    +	for (_i = 0; _i < 8; _i++) { \
    +		((unsigned char *) (v))[_i] = ((unsigned char *) (x))[_i]; \
    +	} \
    +} while(0)
    
     /* getters */
     #define DUK_TVAL_GET_BOOLEAN(v)             ((int) (v)->us[DUK_DBL_IDX_US1])
    @@ -31536,7 +31541,15 @@
     		           new_e_pv != NULL && new_e_f != NULL);
    
     		new_e_k[new_e_used] = key;
    +#if 0
     		new_e_pv[new_e_used] = DUK_HOBJECT_E_GET_VALUE(obj, i);
    +#endif
    +		{
    +			unsigned char *p_dst = (unsigned char *) &new_e_pv[new_e_used];
    +			unsigned char *p_src = (unsigned char *) DUK_HOBJECT_E_GET_VALUE_PTR(obj, i);
    +			volatile int _i;
    +			for (_i = 0; _i < 8; _i++) { p_dst[_i] = p_src[_i]; }
    +		}
     		new_e_f[new_e_used] = DUK_HOBJECT_E_GET_FLAGS(obj, i);
     		new_e_used++;
     	}    
- - 2013-12-10 10:00:44.425108 Z
  - sva <sami.vaarala@iki.fi>
  - commented
  - "See: misc/clang_aliasing.c"
- - 2013-12-10 18:19:25.100543 Z
  - sva <sami.vaarala@iki.fi>
  - commented
  - |-
    The root cause is the fact the clang ends up using a floating point load and
    store to do the structured union assignment.  A load+store sequence on X86
    may convert a signaling NaN into a quiet NaN, which happens by setting the
    highest bit of the mantissa.  See, for instance:
    
        http://caml.inria.fr/mantis/view.php?id=5038
    
    This may be impossible to catch at compile time, so perhaps add a few self
    tests and/or add an assert to DUK_TVAL_SET_TVAL.  There is no telling what
    other code might be affected though.    
- - 2013-12-10 21:35:38.936057 Z
  - sva <sami.vaarala@iki.fi>
  - assigned to release v0.9 from v0.8
  - ""
- - 2014-01-12 14:03:11.829583 Z
  - sva <sami.vaarala@iki.fi>
  - assigned to release v0.10 from v0.9
  - ""
- - 2014-02-11 22:37:38.045628 Z
  - sva <sami.vaarala@iki.fi>
  - assigned to release v0.11 from v0.10
  - ""
- - 2014-02-18 13:22:36.557661 Z
  - sva <sami.vaarala@iki.fi>
  - unassigned from release v0.11
  - ""