[jira] [Updated] (OFBIZ-9679) [FB] Package org.apache.ofbiz.base.conversion

["Dennis Balkir (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/OFBIZ-9679?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Dennis Balkir updated OFBIZ-9679:
---------------------------------
    Attachment: OFBIZ-9679_org.apache.ofbiz.base.conversion_bugfixes.patch

- 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}}
- Line 154: added a catch for a {{RuntimeException}}

> [FB] Package org.apache.ofbiz.base.conversion
> ---------------------------------------------
>
>                 Key: OFBIZ-9679
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-9679
>             Project: OFBiz
>          Issue Type: Sub-task
>          Components: base
>    Affects Versions: Trunk
>            Reporter: Dennis Balkir
>            Priority: Minor
>         Attachments: OFBIZ-9679_org.apache.ofbiz.base.conversion_bugfixes.patch
>
>
> - 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.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)