Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Minor
    • Resolution: Implemented
    • Affects Version/s: Trunk
    • Fix Version/s: Upcoming Release
    • Component/s: base
    • Labels:
      None

      Description

      • BooleanConverters.java:72, DM_CONVERT_CASE

      Dm: Use of non-localized String.toUpperCase() or String.toLowerCase() in org.apache.ofbiz.base.conversion.BooleanConverters$StringToBoolean.convert(String)

      A String is being converted to upper or lowercase, using the platform's default encoding. This may result in improper conversions when used with international characters. Use the

      String.toUpperCase( Locale l )
      String.toLowerCase( Locale l )

      versions instead.

      • CollectionConverters.java:46, BC_VACUOUS_INSTANCEOF

      BC: instanceof will always return true for all nonnull values in org.apache.ofbiz.base.conversion.CollectionConverters$ArrayCreator.createConverter(Class, Class), since all Class are instances of Object

      This instanceof test will always return true (unless the value being tested is null). Although this is safe, make sure it isn't an indication of some misunderstanding or some other logic error. If you really want to test the value for being null, perhaps it would be clearer to do better to do a null test rather than an instanceof test.

      • Converters.java:39, MS_MUTABLE_COLLECTION_PKGPROTECT

      Field is a mutable collection which should be package protected

      A mutable collection instance is assigned to a final static field, thus can be changed by malicious code or by accident from another package. The field could be made package protected to avoid this vulnerability. Alternatively you may wrap this field into Collections.unmodifiableSet/List/Map/etc. to avoid this vulnerability.

      • Converters.java:40, MS_MUTABLE_COLLECTION_PKGPROTECT

      Field is a mutable collection which should be package protected

      A mutable collection instance is assigned to a final static field, thus can be changed by malicious code or by accident from another package. The field could be made package protected to avoid this vulnerability. Alternatively you may wrap this field into Collections.unmodifiableSet/List/Map/etc. to avoid this vulnerability.

      • Converters.java:154, REC_CATCH_EXCEPTION

      REC: Exception is caught when Exception is not thrown in org.apache.ofbiz.base.conversion.Converters.loadContainedConverters(Class)

      This method uses a try-catch block that catches Exception objects, but Exception is not thrown within the try block, and RuntimeException is not explicitly caught. It is a common bug pattern to say try

      { ... }

      catch (Exception e)

      { something }

      as a shorthand for catching a number of types of exception each of whose catch blocks is identical, but this construct also accidentally catches RuntimeException as well, masking potential bugs.

      A better approach is to either explicitly catch the specific exceptions that are thrown, or to explicitly catch RuntimeException exception, rethrow it, and then catch all non-Runtime Exceptions, as shown below:

      try

      { ... }

      catch (RuntimeException e)

      { throw e; }

      catch (Exception e)

      { ... deal with all non-runtime exceptions ... }
      • MiscConverters.java:90, PZLA_PREFER_ZERO_LENGTH_ARRAYS

      PZLA: Should org.apache.ofbiz.base.conversion.MiscConverters$ByteBufferToByteArray.convert(ByteBuffer) return a zero length array rather than null?

      It is often a better design to return a length zero array rather than a null reference to indicate that there are no results (i.e., an empty list of results). This way, no explicit check for null is needed by clients of the method.

      On the other hand, using null to indicate "there is no answer to this question" is probably appropriate. For example, File.listFiles() returns an empty list if given a directory containing no files, and returns null if the file is not a directory.

        Activity

        Hide
        Dennis Balkir Dennis Balkir added a comment - - edited
        • Diamond Operators fixed

        class BooleanConverters:

        • Line 72: added a default Locale to toUpperCase

        class CollectionConverters:

        • Line 46: changed the not instanceof object check to a null-check which seems like it was intended to be

        class Converters:

        • Line 39, 40: changed the parameters from protected to private
        Show
        Dennis Balkir Dennis Balkir added a comment - - edited Diamond Operators fixed class BooleanConverters: Line 72: added a default Locale to toUpperCase class CollectionConverters: Line 46: changed the not instanceof object check to a null-check which seems like it was intended to be class Converters: Line 39, 40: changed the parameters from protected to private
        Hide
        mbrohl Michael Brohl added a comment -

        Thanks Dennis,

        your patch is in trunk r1811434.

        Show
        mbrohl Michael Brohl added a comment - Thanks Dennis, your patch is in trunk r1811434.

          People

          • Assignee:
            mbrohl Michael Brohl
            Reporter:
            Dennis Balkir Dennis Balkir
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development