fix: improve handling of getBoolean and setObject

[jorsol]

This merge the code used to cast to boolean in getBoolean and setObject, it allows only valid values and reject wrong values.

This follows the specification more close and proper support the boolean types of PostgreSQL.

[codecov-io]

Current coverage is 64.41% (diff: 94.02%)

Merging #732 into master will increase coverage by 0.07%

@@             master       #732   diff @@
==========================================
  Files           163        164     +1   
  Lines         15140      15144     +4   
  Methods           0          0          
  Messages          0          0          
  Branches       2987       2979     -8   
==========================================
+ Hits           9741       9755    +14   
+ Misses         4159       4157     -2   
+ Partials       1240       1232     -8   

Powered by Codecov. Last update ee51dfc...700a747

[vlsi]

I wonder if adding actual "bad" value to the error message would count as a security violation.
I think adding the value explicitly would simplify the debugging.

Any thoughts?

[davecramer]

On 24 January 2017 at 17:22, Vladimir Sitnikov ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pgjdbc/src/main/java/org/postgresql/jdbc/BooleanTypeUtil.java <#732 (review)>: > + } + + private static boolean from(final String strval) throws PSQLException { + // Leading or trailing whitespace is ignored, and case does not matter. + final String val = strval.trim(); + if ("1".equals(val) || "true".equalsIgnoreCase(val) + || "t".equalsIgnoreCase(val) || "yes".equalsIgnoreCase(val) + || "y".equalsIgnoreCase(val) || "on".equalsIgnoreCase(val)) { + return true; + } + if ("0".equals(val) || "false".equalsIgnoreCase(val) + || "f".equalsIgnoreCase(val) || "no".equalsIgnoreCase(val) + || "n".equalsIgnoreCase(val) || "off".equalsIgnoreCase(val)) { + return false; + } + throw cannotCastException("String"); I wonder if adding actual "bad" value to the error message would count as a security violation. I think adding the value explicitly would simplify the debugging. Any thoughts?

The only way it might be a security problem is if the exception wasn't caught. This is the programmers problem IMO Dave Cramer

…

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#732 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYz9p83KYdtC4vLXIMCsxi_-tdmhT0Mks5rVnmJgaJpZM4LlA2V> .

[vlsi]

The only way it might be a security problem is if the exception wasn't
caught. This is the programmers problem IMO

In that case I think it makes sense to include the value to the exception, so programmer can understand and fix the problem.

[jorsol]

@vlsi , Apart from the value in the exception, I also included the logger so it's now even easier to debug and fix the problem. I also improve the boolean tests and included an appropriate javadoc to getBoolean.

[vlsi]

@jorsol , did you check performance implications of adding a debug logging to a success path?

Quick glance suggests it is not free: http:https://stackoverflow.com/a/39748222/1261287

I don't think there were similar cases when "getXXX" involved memory allocation through logger.log(, new Object[]{...})

I'm inclined to remove that kind of debug logging from the hot path.
I would be fine if array allocation was guarded by the relevant isLoggable check.

[jorsol]

@vlsi , I checked now, and yes, there is a performance penalty, I simplify it to still be able to debug the value and don't hit the performance penalty when logging is off...

BTW there is always a performance penalty of having the logger enabled, but only if the level allows it. That's why in production its not recommended to have logging enabled or with a DEBUG (FINE) level, and it usage is only for that "debug".

        if (!isLoggable(level)) {
            return;
        }