Subject: [Gen-art] Gen-ART LC Review of draft-ietf-rohc-sigcomp-impl-guide-09.txt

From: "Spencer Dawkins" <[email-masked]>
Date: Wed, 20 Dec 2006 10:36:43 -0600

I have been selected as the General Area Review Team (Gen-ART)
reviewer for this draft (for background on Gen-ART, please see
http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-rohc-sigcomp-impl-guide-09.txt
Reviewer: Spencer Dawkins
Review Date:  2006-12-18
IETF LC End Date: 2006-12-21
IESG Telechat date: (if known)

Summary: This document is almost ready for publication as a Proposed
Standard, although I do have questions and comments for the authors.

Comments: The questions and comments in Section 2.2, 3.1, 3.2, 3.4, 4, 4.1
should be addressed as part of Last Call comment resolution. Other items,
marked as "Nit -", may be fixed in any editing pass.

Abstract

   This document describes common misinterpretations and some
   ambiguities in the Signalling Compression Protocol (SigComp), and
   offers guidance to developers to clarify any resultant problems.

Spencer: Nit - "clarify any resultant problems" seems strange - "clarify any
resultant confusion", or "resolve any resultant problems"?

   SigComp defines a scheme for compressing messages generated by
   application protocols such as the Session Initiation Protocol (SIP).
   This document (if approved) updates: RFC 3320, RFC 3321, RFC 3485.

Spencer: Nit - IETF is not entirely consistent about whether implementation
guides simply clarify, or if they correct as well. It might be helpful to
say "clarifies and corrects text in the following updated RFCs".

2.1  Bytecode within Decompression Memory Size

   SigComp [1] states that the default Decompression Memory Size (DMS)
   is 2K. The UDVM memory size is defined in section RFC3320:7 to be
   (DMS - sizeof (sigcomp_msg)) for messages transported over UDP and
   (DMS / 2) for those transported over TCP.  This means that when the
   message contains the bytecode (as it will for at least the first
   message) there will actually be two copies of the bytecode within the
   decompressor memory (see Figure 1).  It is correct that there are two
   copies of the bytecode within the decompressor memory, in this case.

Spencer: Nit - the last two sentences were difficult to read because so much
of the text is duplicated. Perhaps "The presence of the second bytecode copy 
is not an error in this case"?

    |<----------------------------DMS--------------------------------->|
    |<-----sigcomp message---->|<------------UDVM memory size--------->|
    +-+----------+-------------+-----+----------+----------------------+
    | | bytecode |  comp msg   |     | bytecode | circular buffer      |
    +-+----------+-------------+-----+----------+----------------------+
     ^                            ^
     |                            |
    Sigcomp header          Low bytes of UDVM

            Figure 1: Bytecode and UDVM memory size within DMS

2.2  Default Decompression Memory Size

   For many implementations, the length of decompression bytecode sent
   is in the range of three to four hundred bytes.  Because SigComp
   specifies a default DMS of 2K, the described scheme seriously
   restricts the size of the circular buffer, and of the compressed
   message itself.  In some cases, this set of circumstances has a
   damaging effect on the compression ratio; for others, it makes it
   completely impossible to send certain messages compressed.

   To address this problem, those mandating the use of Sigcomp need to
   also provide further specification for their application that
   mandates the use of an appropriately sized DMS.

Spencer: I'm concerned that section 2.2 provides a heuristic to identify an
unreasonable length, but not a reasonable one. If someone didn't do the
analysis, would a 1K length be an improvement? a 2K length? If you can
provide any guidance, that would likely be helpful.

3.  UDVM Instructions

3.1  Data Input Instructions

   When inputting data from the compressed message, the INPUT-BYTES
   (section RFC3320-9.4.2) and INPUT-BITS (section RFC3302-9.4.3)
   instructions both have the paragraph:

   "If the instruction requests data that lies beyond the end of the
   SigComp message, no data is returned.  Instead the UDVM moves program
   execution to the address specified by the address operand."

   The intent is that if n bytes/bits are requested but only m are left
   in the message (where m < n) then no bytes/bits MUST be returned to
   the UDVM and the m bytes/bits that are there MUST remain in the
   message unchanged.

Spencer: the first MUST doesn't seem clear - perhaps "the decompressor
dispatcher MUST NOT return any bytes/bits to the UDVM, and the m bytes/bits
available MUST remain in the message unchanged"? But "no bytes MUST be
returned" isn't "MUST NOT return any BYTES, IIUC.

   For example, if the remaining bytes of a message are: 0x01 0x02 0x03
   and the UDVM encounters an INPUT-BYTES (6, a, b) instruction.  The
   decompressor dispatcher returns no bytes and jumps to the instruction
   specified by b.  This contains an INPUT-BYTES (2, c, d) instruction
   so the decompressor dispatcher successfully returns the bytes 0x01
   and 0x02.

   In the case where an INPUT-BYTES instruction follows an INPUT-BITS
   instruction that has left a partial byte in the message, the partial
   byte should still be thrown away even if there are not enough bytes
   to input.

   INPUT-BYTES (0, a, b) can be used to flush out a partial byte.

3.2  MULTILOAD

Spencer: The description of MULTILOAD isn't completely clear to me (I THINK
I understand what is being described, but "not allowed to be self modifying"
is ambiguous). It would be clearer to me, I think, if the first sentence was
"Any indirection of MULTILOAD parameters must be done at execution time, in
order to make step-by-step implementation simpler", and the "That is"
sentence at the end of the paragraph was deleted. Is this what the paragraph
is trying to say?

   The MULTILOAD instruction is explicitly not allowed to be self
   modifying in order to make step-by-step implementation simpler.
   Therefore, any other implementation technique (e.g. decode all
   operands then execute, which is the model of all other instructions)
   MUST yield the same result as a step-by-step implementation would.
   That is, if there is any indirection of parameters, the indirection
   MUST be done at execution time.  For example:

3.4  Using the stack

   The instructions PUSH, POP, CALL and RETURN make use of a stack which
   is set up using the well known memory address stack_location to
   define where in memory the stack is located.  Use of the stack is
   defined in section RFC3320-8.3 which states that: '"Pushing" a value
   on the stack is an abbreviation for copying the value to
   stack[stack_fill] and then increasing stack_fill by 1.' and
   'stack_fill is an abbreviation for the 2-byte word at stack_location
   and stack_location + 1'.  In the very rare case that the value of
   stack_fill is 0xFFFF, the new value pushed onto the stack will be

Spencer: this text describes a problem and almost obscures the solution.
Suggest "In the very rare case that the value of stack_fill is 0xFFFF, the
original stack_fill value should be increased by 1 to 0x0000 and written
back to stack_location + 1 (which will overwrite the value that has been
pushed onto the stack".

Spencer: I'm confused about why some of this text is "MUST (NOT)", but
"increased by 1 to 0x0000" is only a "should" - and isn't 2119 language.

   written to stack [0xFFFF] = stack_location.  Stack_fill would then be
   increased by 1, however, the value at stack_location and
   stack_location + 1 has just been updated.  To maintain the integrity
   of the stack with regard to over and underflow, stack_fill MUST NOT
   be re-read.  The original stack_fill value of 0xFFFF should be
   increased by 1 to 0x0000 and written back to stack_location and
   stack_location + 1 (which will overwrite the value that has been
   pushed onto the stack).

4.  Byte Copying Rules

   Section RFC3320-8.4 states that "The string of bytes is copied in
   ascending order of memory address, respecting the bounds set by

Spencer: the following text also seems misleading :-) or at least confusing.
Is it saying that  attempts to copy bytes outside the bounds will still
result in circular buffer references within the UDVM memory, so they are OK,
or something else? I'm not seeing a clear statement of what happens when I
copy to the left of byte_copy_left, etc.

   byte_copy_left and byte_copy_right."  This is misleading in that it
   is perfectly legitimate to copy bytes outside of the bounds set by
   byte_copy_left and byte_copy_right.  Byte_copy_left and
   byte_copy_right provide the ability to maintain a circular buffer as
   follows:

   For moving to the right

   if current_byte == ((byte_copy_right - 1) mod 2 ^ 16):
       next_byte = byte_copy_left
   else:
       next_byte = (current_byte + 1) mod 2 ^ 16

   which is equivalent to the algorithm given in section 8.4.

   For moving to the left

   if current_byte == byte_copy_left:
       previous_byte = (byte_copy_right - 1) mod 2 ^ 16
   else:
       previous_byte = (current_byte - 1) mod 2 ^ 16

   Moving to the left is only used for COPY_OFFSET.

   Consequently, copying could begin to the left of byte_copy_left and
   continue across it (and jump back to it according to the given
   algorithm if necessary) and could begin at or to the right of
   byte_copy_right (though care must be taken to prevent decompression
   failure due to writing to / reading from beyond the UDVM memory).

4.1  Instructions That Use Byte Copying Rules

   Section RFC3320-8.4 specifies the byte copying rules and includes a
   list of the instructions that obey them.  STATE-CREATE is not in this
   list but END-MESSAGE is.  This caused confusion due to the fact that
   neither instruction actually does any byte copying, rather both
   instructions give information to the state-handler to create state.
   Logically both instructions should have the same information about
   byte copying.

Spencer: so is this text saying that STATE-CREATE should be in the list? Or
something else?

   When state is created by the state-handler (whether the instruction
   was from END-MESSAGE or STATE-CREATE), the byte copying rules of
   section RFC3320-8.4 apply.

   Note that, if the contents of the UDVM changes between the occurrence
   of the STATE-CREATE instruction and the state being created, the
   bytes that are stored are those in the buffer at the time of creation
   (i.e. when the message has been decompressed and authenticated).

   CRC is not mentioned in section RFC3320-8.4 in the list of
   instructions that obey byte copying rules, but its description in
   section RFC3320-9.3.5 states that these rules are to be obeyed.  When
   reading data over which to perform the crc check, byte copying rules
   apply as specified in section RFC3320-8.4.

Spencer: again - is this text saying that CRC should be in the list?

   When the partial identifier for a STATE-FREE instruction is read,
   (during the execution of END-MESSAGE) byte copying rules as per
   section RFC3320-8.4 apply.

12.  RFC 3485 SIP/SDP Static Dictionary

   However, there are some minor bugs in the dictionary.  In a number of
   places, the entry in the offset table refers to an address that is
   not in the corresponding priority section in the list of strings.
   Consequently, if the bytecode uses the offset table and limits use of
   the dictionary to priorities less than 4, then care must be taken not
   to use the following strings in the dictionary:

      'application' at 0x0334 is not at priority 2 (it's priority 4)
      'sdp' at 0x064b is not at priority 2 (it's priority 4)
      'send' at 0x089d is not at priority 2 (it's priority 3)
      'recv' at 0x0553 is not at priority 2 (it's priority 4)
      'phone' at 0x00f2 is not at priority 3 (it's priority 4)

   These are seen to be relatively low cost bugs as only these 5 strings
   are affected and they are only affected under certain conditions.

Spencer: Nit - I can't make myself object to documenting this caution
instead of updating the dictionary specification (here or elsewhere), but I
wish I could... :-( Is the assumption that it's not worth the disruption of
bumping the version number?

15.  Acknowledgements

Spencer: Nit - the first sentence here isn't clear about who is being
foolish (and it's a nice comment that needs to be clear in order to be
clever).

   Largely through being foolish enough to be authors of, or to have
   implemented, SigComp we would like to thank the following:
      Richard Price
      Lajos Zaccomer
      Timo Forsman
      Tor-Erik Malen
      Jan Christoffersson
      Kwang Mien Chan
      William Kembery
      Pekka Pessi
   for their confusion, suggestions and comments.

Appendix A.  Dummy Application Protocol (DAP)

A.1  Introduction

   This appendix defines a simple dummy application protocol (DAP) that
   can be used for SigComp interoperability testing.  This is handy for
   SigComp implementations that are not integrated with SIP stack.  It
   also provides some features that facilitate the tests of SigComp
   internal operations.

Spencer: thank you guys for including this. I wish all our protocol specs
were this well-defined ;-}

_______________________________________________
Gen-art mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/gen-art