ofbiz-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacques Le Roux <jacques.le.r...@les7arts.com>
Subject Re: svn commit: r1813964 - /ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbi z/entity/util/EntityCrypto.java
Date Tue, 07 Nov 2017 13:10:03 GMT
Hi Deepak,

The problem as I see it is that if a RuntimeException is thrown by the 1st version of doDecrypt()
then maybe the 2nd version of doDecrypt() will do 
so. Maybe I'm wrong here, I did not check each method body.

Actually I think it's more a theoretical issue than a real one because among the possible
the possible RuntimeException subclasses[1], even if I did 
not check them all, I hardly see one being thrown here. But maybe a NPE in some cases? In
another word is more to follow a pattern and use it 
everywhere in our code than about this peculiar issue. You may revert if you want but beware
of NPE ;)

Jacques
[1] https://docs.oracle.com/javase/7/docs/api/java/lang/RuntimeException.html


Le 07/11/2017 à 11:19, Deepak Dixit a écrit :
> Hi Jacques,
>
> I think we don't need to handle the RuntimeException, as doDecrypt fails to
> decrypt then it will try with old pattern, I think this is for backward
> compatibility.
>
> Here is the comment from code
> try using the old/bad hex encoding approach; this is another path the code
> may take, ie if there is an exception thrown in decrypt
>
>
> Thanks & Regards
> --
> Deepak Dixit
> www.hotwaxsystems.com
> www.hotwax.co
>
> On Fri, Nov 3, 2017 at 5:15 PM, Jacques Le Roux <
> jacques.le.roux@les7arts.com> wrote:
>
>> Hi Deepak,
>>
>> Actually the rule is quite simple. If, for a reason, you decide to catch a
>> java.lang.Exception, you need to catch before the
>> java.lang.RuntimeException.
>>
>> This is because java.lang.RuntimeException is a java.lang.Exception so you
>> would hide possible runtime issues by only catching Exception
>>
>> https://www.tutorialspoint.com/java/images/exceptions1.jpg
>>
>> HTH
>>
>> Jacques
>>
>>
>> Le 03/11/2017 à 11:17, Deepak Dixit a écrit :
>>
>>> Thanks Jacques,
>>> I was engage in other work so did not get a chance to test your
>>> suggestion... :)
>>>
>>> Thanks & Regards
>>> --
>>> Deepak Dixit
>>> www.hotwaxsystems.com
>>> www.hotwax.co
>>>
>>> On Fri, Nov 3, 2017 at 3:19 PM, Jacques Le Roux <
>>> jacques.le.roux@les7arts.com> wrote:
>>>
>>> Done at r1814155
>>>> Jacques
>>>>
>>>> Le 01/11/2017 à 14:08, Jacques Le Roux a écrit :
>>>>
>>>> Hi Deepak,
>>>>
>>>> It's minor, but instead of hiding a possible RuntimeException by catching
>>>> Exception here I'd rather follow this FindBugs advice
>>>> ------------------------------
>>>> 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 ...}
>>>> ------------------------------
>>>>
>>>>
>>>> I suggest to use this late solution, as it has for example been done for
>>>> GroovyUtil.java in r1812059
>>>>
>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
>>>> framework/base/src/main/java/org/apache/ofbiz/base/util/
>>>> GroovyUtil.java?r1=1812059&r2=1812058&pathrev=1812059
>>>>
>>>> Thanks
>>>>
>>>> Jacques
>>>>
>>>> Le 01/11/2017 à 11:43, deepak@apache.org a écrit :
>>>>
>>>> Author: deepak
>>>> Date: Wed Nov  1 10:43:14 2017
>>>> New Revision: 1813964
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1813964&view=rev
>>>> Log:
>>>> Fixed: doDecrypt method may return ClassNotFoundException,
>>>> BadPaddingException,so instead of handling GeneralException used Exception
>>>> class to handle all exception
>>>>
>>>> Modified:
>>>>       ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/
>>>> org/apache/ofbiz/entity/util/EntityCrypto.java
>>>>
>>>> Modified: ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/
>>>> org/apache/ofbiz/entity/util/EntityCrypto.java
>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
>>>> mework/entity/src/main/java/org/apache/ofbiz/entity/util/
>>>> EntityCrypto.java?rev=1813964&r1=1813963&r2=1813964&view=diff
>>>> ============================================================
>>>> ==================
>>>> --- ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/
>>>> org/apache/ofbiz/entity/util/EntityCrypto.java (original)
>>>> +++ ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/
>>>> org/apache/ofbiz/entity/util/EntityCrypto.java Wed Nov  1 10:43:14 2017
>>>> @@ -124,7 +124,7 @@ public final class EntityCrypto {
>>>>        public Object decrypt(String keyName, EncryptMethod encryptMethod,
>>>> String encryptedString) throws EntityCryptoException {
>>>>            try {
>>>>                return doDecrypt(keyName, encryptMethod, encryptedString,
>>>> handlers[0]);
>>>> -        } catch (GeneralException e) {
>>>> +        } catch (Exception e) {
>>>>                Debug.logInfo("Decrypt with DES key from standard key name
>>>> hash failed, trying old/funny variety of key name hash", module);
>>>>                for (int i = 1; i < handlers.length; i++) {
>>>>                    try {
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>


Mime
View raw message