723461 - ByteArray readUTFBytes does not properly update position when it starts at a UTF8 BOM

Status: RESOLVED FIXED

(Tamarin Graveyard :: Library, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: code illustrating issue

Spawned off from Bug 707700.

Code:
  import flash.utils.ByteArray;
  var b:ByteArray = new ByteArray();
  var s1:String = '\ufeff1234'
  b.writeUTFBytes(s1);
  print("s1: "+s1+" s1.length: "+s1.length);
  print("b.length: "+b.length+" b.position: "+b.position);
  b.position = 0;
  print("reseting to start, b.position: "+b.position);
  var len = b.length;
  var s = b.readUTFBytes(len);
  print("after reading "+len+" bytes, b.position: "+b.position);

Prints:
  s1: 1234 s1.length: 5
  b.length: 7 b.position: 7
  reseting to start, b.position: 0
  after reading 7 bytes, b.position: 4

Note that in a sane world, reading 7 bytes would move the position forward by 7 bytes.

I suspect this is due to the following attempt to "compensate" for a BOM in the code for ByteArrayGlue::ReadUTFBytes:

    String* ByteArrayObject::readUTFBytes(uint32_t length)
    {
        if (m_byteArray.Available() < length)
            toplevel()->throwEOFError(kEOFError);

        const uint8_t* p = (const uint8_t*)m_byteArray.GetReadableBuffer() + m_byteArray.GetPosition();
        // Skip UTF8 BOM (but it is still counted in the length we consume).
        if (length >= 3 && p[0] == 0xEFU && p[1] == 0xBBU && p[2] == 0xBFU)
        {
            p += 3;
            length -= 3;
        }

        ...

        // The position is always updated as if the entire string had been consumed, 
        // even if there was a NUL that made us stop early.
        m_byteArray.SetPosition(m_byteArray.GetPosition()+length);

        return result;
    }

And since the position is not updated correctly, most of the ByteArray API will not work properly until the position is explicitly updated.

Given how incredibly broken this is, I suspect this compensation for the BOM must not be something we are actually encountering in practice, and could perhaps be dropped.  But I admit that is a more extreme change than updating the code to properly update the position.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #0)
> Given how incredibly broken this is, I suspect this compensation for the BOM
> must not be something we are actually encountering in practice, and could
> perhaps be dropped. [...]

That, or this is a very recent injection, and we don't test the BOM skipping properly.

:(

Guess what, that's what it is; this bug was injected by this commit for Bug 692044, comment 3:

  http://hg.mozilla.org/tamarin-redux/rev/20c048ed1b6e

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch P: update position properly when skipping leading BOM

I plan to push with review pending.

(Should we attempt to get this into Brannan?)

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch R: regression test

tests that:

1. the bug in question is fixed (i.e. that position is updated),
2. subsequent reads from the ByteArray also work properly, and
3. the BOM actually is skipped properly.

Before putting in patch P, I see 1. and 2. failing, like so:

1 running regress/bug_723461.as  
   at 0 result subsequent reads correct = 2345 FAILED! expected: 5678
   at 0 position updated correctly = 4 FAILED! expected: 7
   at 10 result subsequent reads correct = 2345 FAILED! expected: 5678
   at 10 position updated correctly = 4 FAILED! expected: 7
   FAILED passes:2 fails:4 unexpected passes: 0 expected failures: 0

After putting in patch P, the test succeeds.

Comment 4

[Tamarin Bot]

changeset: 7181:c8486cf127a2
user:      Felix S Klock II <[e-mail]>
summary:   Bug 723461: update ByteArray position properly when skipping leading BOM (r pending=edwsmith).

http://hg.mozilla.org/tamarin-redux/rev/c8486cf127a2

Comment 5

[Tamarin Bot]

changeset: 7182:5bec038b8b7b
user:      Felix S Klock II <[e-mail]>
summary:   Bug 723461: regression test (r pending=dschaffe).

http://hg.mozilla.org/tamarin-redux/rev/5bec038b8b7b

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

(I've pushed the patches but I will wait until the reviews to change the status to Fixed or otherwise.)

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

QRB: should this be pushed to Brannan?

Comment 8

[William Maddox]

Submitted to Brannan, CL 1030786 .

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to William Maddox from comment #8)
> Submitted to Brannan, CL 1030786 .

Thanks Bill!

(In hindsight I should probably have submitted my patch to FR Main as well as TR; that might have made your life easier, in terms of making the Brannan submission a direct p4 integrate command.)

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

Submitted to FR Main, CL 1031267 .