Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: Classlib
    • Labels:
      None
    • Estimated Complexity:
      Moderate

      Description

      Let's consider the following test:

      import java.text.Bidi;
      public class Test {
      public static void main(String[] args) throws Exception {
      Bidi bd = new Bidi(new char[]

      { 's', 's', 's' }

      , 0,
      new byte[]

      { (byte) -7, (byte) -2, (byte) -3 }

      ,
      0, 3, Bidi.DIRECTION_DEFAULT_LEFT_TO_RIGHT);
      System.out.println("Expected 7, real " + " " + bd.getLevelAt(0));
      }
      }

      In my opinion the JNI implementation of Bidi (text/BidiWrapper.c file, ubidi_1setPara() function) contains a potential bug, namely:
      1. If the embeddingLevels argument is not NULL then _embeddingLevels variable is initialized with the JNI GetByteArrayElements() function;
      2. ICU function (ubidi_setPara) initializes ICU inner structure and puts the _embeddingLevels into it;
      3. If _embeddingLevels pointer is not NULL then the JNI ReleaseByteArrayElements() function (with 0 as fourth parameter) is called.
      This function releases the memory (according to JNI specification) the _embeddingLevels pointer refers to;
      4. After that ICU inner structure isn't initialized properly. Call of ICU ubidi_getLevels() function can return incorrect values (see java test above).

      It seems the JNI_COMMIT parameter instead of "0" should be passed to the ReleaseByteArrayElements() to avoid this problem.

      I'd like to underline the test mentioned above works w/o any issues for Harmony-14 contribution (although it shouldn't sometimes IMHO).
      Therefore if there are any doubts in my argumentation this issue can be closed as invalid.

      1. HARMONY-100.diff
        6 kB
        Alexey Petrenko
      2. HARMONY-100.diff
        6 kB
        Alexey Petrenko

        Activity

        Hide
        Tim Ellison added a comment -

        Vladimir,

        Thanks for the observation. I agree there is a problem here, but I don't agree with your conclusion.

        Looking at
        http://icu.sourceforge.net/apiref/icu4c/ubidi_8h.html#a22
        especially the paragraph beginning "Caution: A copy of this pointer...".

        and the native code in
        Java_org_apache_harmony_text_BidiWrapper_ubidi_1setPara

        The Harmony code passes in the address of a local variable '_embeddingLevels' which, as you point out will go out of scope at the end of this function (precisely the case cautioned against). If the JNI impl is data-copying then obviously this can lead to subsequent invalid access, and if the JNI-impl is pinning then GC could move the byte array upon release of the data.

        However, I don't see how committing the memory back to the java object helps? The memory needs to be stable until the call to Java_org_apache_harmony_text_BidiWrapper_ubidi_1close. Right now it is java memory allocated in the BiDiWrapper:

        private static long createUBiDi(char[] text, int textStart,
        byte[] embeddings, int embStart, int paragraphLength, int flags) {
        char[] realText = null;

        byte[] realEmbeddings = null;

        I think the solution is to allocate this realEmbeddings memory as OSMemory in Java and pass the address into the native call, or allocating it as VM static memory within the native code itself, then freeing it after closing the wrapper.

        Show
        Tim Ellison added a comment - Vladimir, Thanks for the observation. I agree there is a problem here, but I don't agree with your conclusion. Looking at http://icu.sourceforge.net/apiref/icu4c/ubidi_8h.html#a22 especially the paragraph beginning "Caution: A copy of this pointer...". and the native code in Java_org_apache_harmony_text_BidiWrapper_ubidi_1setPara The Harmony code passes in the address of a local variable '_embeddingLevels' which, as you point out will go out of scope at the end of this function (precisely the case cautioned against). If the JNI impl is data-copying then obviously this can lead to subsequent invalid access, and if the JNI-impl is pinning then GC could move the byte array upon release of the data. However, I don't see how committing the memory back to the java object helps? The memory needs to be stable until the call to Java_org_apache_harmony_text_BidiWrapper_ubidi_1close. Right now it is java memory allocated in the BiDiWrapper: private static long createUBiDi(char[] text, int textStart, byte[] embeddings, int embStart, int paragraphLength, int flags) { char[] realText = null; byte[] realEmbeddings = null; I think the solution is to allocate this realEmbeddings memory as OSMemory in Java and pass the address into the native call, or allocating it as VM static memory within the native code itself, then freeing it after closing the wrapper.
        Hide
        Tim Ellison added a comment -

        What I forgot to say at the end was... and thank you for reading it so closely to find this bug!

        Show
        Tim Ellison added a comment - What I forgot to say at the end was... and thank you for reading it so closely to find this bug!
        Hide
        Alexei Fedotov added a comment -
        Show
        Alexei Fedotov added a comment - [drlvm] [unit] Blocks http://wiki.apache.org/harmony/Unit_Tests_Pass_on_DRLVM
        Hide
        Alexey Petrenko added a comment -

        I agree with Tim's suggestion and will create a patch.

        Show
        Alexey Petrenko added a comment - I agree with Tim's suggestion and will create a patch.
        Hide
        Alexey Petrenko added a comment -

        It seems that it is not possible to retrieve original embedding levels pointer from UBiDi. So we need to store it somewhere.

        I've introduced a BiDiData structure to keep UBiDi and embedding levels pointers at the same place. In this case only native part is changed.

        NB: This issue causes BiDiTest failure on DRLVM while it works fine on IBM VME. So we do not need additional unit tests.

        Show
        Alexey Petrenko added a comment - It seems that it is not possible to retrieve original embedding levels pointer from UBiDi. So we need to store it somewhere. I've introduced a BiDiData structure to keep UBiDi and embedding levels pointers at the same place. In this case only native part is changed. NB: This issue causes BiDiTest failure on DRLVM while it works fine on IBM VME. So we do not need additional unit tests.
        Hide
        Alexey Varlamov added a comment -

        Hmm, using malloc() does not look consistent given the portlib existence.
        Could you please consider the following usage example:

        PORT_ACCESS_FROM_ENV (env);
        BiDiData *data = (BiDiData *) hymem_allocate_memory (sizeof(BiDiData));
        instead of
        BiDiData *data = (BiDiData *)malloc(sizeof(BiDiData));

        Show
        Alexey Varlamov added a comment - Hmm, using malloc() does not look consistent given the portlib existence. Could you please consider the following usage example: PORT_ACCESS_FROM_ENV (env); BiDiData *data = (BiDiData *) hymem_allocate_memory (sizeof(BiDiData)); instead of BiDiData *data = (BiDiData *)malloc(sizeof(BiDiData));
        Hide
        Alexey Petrenko added a comment -

        Here is modified patch which is using portlib.

        Show
        Alexey Petrenko added a comment - Here is modified patch which is using portlib.
        Hide
        Alexey Petrenko added a comment -

        Patch is applied.

        Show
        Alexey Petrenko added a comment - Patch is applied.

          People

          • Assignee:
            Alexey Petrenko
            Reporter:
            Vladimir Gorr
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development