Details

    • Bug
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • 2.2.5
    • 2.2.6, 3.0
    • Openwire
    • None
    • Windows XP SP 3, Visual Studio 2008

    • Regression

    Description

      Hallo,

      we are using topic messages to sent messages from one user to another. Our program subscribe a durable consumer with selector "UserName='<user>'" and send a message with the property "UserName" and value "<user>".

      All works fine, when <user> contains only ASCII characters. When <user> contains non ASCII characters like äöüßé, the message is not send to the
      consumer.

      The problem ist that readString and writeString in OpenwireStringSupport.cpp have bugs

      Regards,
      Peter

      Attachments

        1. OpenwireStringSupportTest.patch
          4 kB
          Martin Schlapfer
        2. OpenwireStringSupport.patch
          6 kB
          Peter Pfort
        3. OpenwireStringSupport_fixed.patch
          7 kB
          Martin Schlapfer

        Activity

          retep Peter Pfort added a comment -

          Hi,

          here is the changed src\main\activemq\connector\openwire\utils\OpenwireStringSupport.cpp.

          Peter

          retep Peter Pfort added a comment - Hi, here is the changed src\main\activemq\connector\openwire\utils\OpenwireStringSupport.cpp. Peter

          When attaching a fix its much nicer if you could attach a patch file as its easier to apply to the different branches of code, also I can't do anything until you resubmit your patch and select to grant the code to the ASF license.

          tabish Timothy A. Bish added a comment - When attaching a fix its much nicer if you could attach a patch file as its easier to apply to the different branches of code, also I can't do anything until you resubmit your patch and select to grant the code to the ASF license.

          Patch applied, thanks!

          tabish Timothy A. Bish added a comment - Patch applied, thanks!

          Tim/Peter,

          Having just worked in this area on another piece of software, I have a few code review comments on this patch:

          (1) In the readString method, the transformation from UTF8 to Unicode should be limited to 1 byte (max value 255) since the UTF8 data is decoded in this method into a byte array. Decoding values above 255 and stuffing the value into a byte value will only cause debugging problems down then road when receiving UTF8 data with values above 255. Thus, as with the code before patch was applied (with values above 127) , the readString method should throw an IO Exception if a Unicode value greater than 255 is encountered indicating "Encoding is not supported".

          (2) For performance reasons (although not much of a factor with 1 byte Unicode, however greater factor in supporting 2 byte and 4 byte Unicode), bitwise operators should be used to decode / encode between UTF8 and Unicode rather than arithmetic.

          (3) The "null character", value 0, should not be skipped. It should be treated as a character and decoded / endcoded along with the rest of the characters. The null character is a valid value in UTF8 and Unicode (and c++ std::string's). The null character is a C style string programming artifact.

          my two cents, thanks,
          Martin.

          mschlapf Martin Schlapfer added a comment - Tim/Peter, Having just worked in this area on another piece of software, I have a few code review comments on this patch: (1) In the readString method, the transformation from UTF8 to Unicode should be limited to 1 byte (max value 255) since the UTF8 data is decoded in this method into a byte array. Decoding values above 255 and stuffing the value into a byte value will only cause debugging problems down then road when receiving UTF8 data with values above 255. Thus, as with the code before patch was applied (with values above 127) , the readString method should throw an IO Exception if a Unicode value greater than 255 is encountered indicating "Encoding is not supported". (2) For performance reasons (although not much of a factor with 1 byte Unicode, however greater factor in supporting 2 byte and 4 byte Unicode), bitwise operators should be used to decode / encode between UTF8 and Unicode rather than arithmetic. (3) The "null character", value 0, should not be skipped. It should be treated as a character and decoded / endcoded along with the rest of the characters. The null character is a valid value in UTF8 and Unicode (and c++ std::string's). The null character is a C style string programming artifact. my two cents, thanks, Martin.

          We welcome additional patches.

          tabish Timothy A. Bish added a comment - We welcome additional patches.

          Reopening to submit patch as mentioned in previous comment.

          mschlapf Martin Schlapfer added a comment - Reopening to submit patch as mentioned in previous comment.

          Updated patch file, per previous comments.

          mschlapf Martin Schlapfer added a comment - Updated patch file, per previous comments.

          Found a problem with readString during testing of encodings containing 1-byte AND 2-byte sequences. For a 1-byte sequence the value was not placed back in the value array. Please use this updated patch instead of the previous I had attached. Thanks.

          mschlapf Martin Schlapfer added a comment - Found a problem with readString during testing of encodings containing 1-byte AND 2-byte sequences. For a 1-byte sequence the value was not placed back in the value array. Please use this updated patch instead of the previous I had attached. Thanks.

          I won't get to this until tomorrow or sat, so feel free to augment the existing CPPUnit test for the code and attach that patch as well.

          Thanks for the contribution.

          tabish Timothy A. Bish added a comment - I won't get to this until tomorrow or sat, so feel free to augment the existing CPPUnit test for the code and attach that patch as well. Thanks for the contribution.

          Updated cppunit test case for OpenwireStringSupportTest to support patch to OpenwireStringSupport.

          mschlapf Martin Schlapfer added a comment - Updated cppunit test case for OpenwireStringSupportTest to support patch to OpenwireStringSupport.

          I've added the patches to the trunk code today, I'll work on adding it to the 2.x branch tomorrow. Great Work! Thanks for the patches. keep them coming

          I also added a small benchmark test for the OpenWireStringSupport class to the benchmarks suite for performance comparisons.

          tabish Timothy A. Bish added a comment - I've added the patches to the trunk code today, I'll work on adding it to the 2.x branch tomorrow. Great Work! Thanks for the patches. keep them coming I also added a small benchmark test for the OpenWireStringSupport class to the benchmarks suite for performance comparisons.

          Resolved, patches added to trunk and 2.x

          tabish Timothy A. Bish added a comment - Resolved, patches added to trunk and 2.x

          People

            tabish Timothy A. Bish
            retep Peter Pfort
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: