[
https://issues.apache.org/jira/browse/ZOOKEEPER-3504?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16905774#comment-16905774
]
Jürgen Wagner commented on ZOOKEEPER-3504:
------------------------------------------
As I have seen this message on the Zookeeper lists now at least twice, I feel
compelled to reply.
The Logger object will only log if debug mode is enabled, anyway. Therefore,
wrapping an SLF4J logger call to LOG.XXX in a LOG.isXXXEnabled() is
functionally redundant, and may only check for the debug mode a little bit
earlier, saving a bit of CPU time to get into the Logger and check, and to
compute TxnHeader.getType().
The only criticism one could apply from my point of view is the use of string
concatenation instead of string pattern arguments, because this way we would
create string garbage even if debug mode is not enabled. However, you can see
this being used at the end of processTransaction(), so you seem to be referring
to an older version of the code.
https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java
Sensitive information will not be leaked as debug mode is not supposed to be
enabled in a production system (apart from the fact that even with the addition
of the proposed check, the same amount of information would be logged).
> An information leakage from FileTxnSnapLog to log:
> --------------------------------------------------
>
> Key: ZOOKEEPER-3504
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-3504
> Project: ZooKeeper
> Issue Type: Bug
> Components: security, server
> Affects Versions: 3.4.11, 3.4.12, 3.4.13, 3.5.5, 3.4.14
> Reporter: xiaoqin.fu
> Priority: Major
>
> In org.apache.zookeeper.server.persistence.FileTxnSnapLog, the statement
> LOG.debug don't have LOG controls:
> public void processTransaction(TxnHeader hdr,DataTree dt,
> Map<Long, Integer> sessions, Record txn)
> throws KeeperException.NoNodeException {
> ......
> if (rc.err != Code.OK.intValue()) {
> LOG.debug("Ignoring processTxn failure hdr:" + hdr.getType()
> + ", error: " + rc.err + ", path: " + rc.path);
> }
> ......
> }
> Sensitive information about hdr type or rc path was leaked. The conditional
> statement LOG.isDebugEnabled() should be added:
> public void processTransaction(TxnHeader hdr,DataTree dt,
> Map<Long, Integer> sessions, Record txn)
> throws KeeperException.NoNodeException {
> ......
> if (rc.err != Code.OK.intValue()) {
> if (LOG.isDebugEnabled())
> LOG.debug("Ignoring processTxn failure hdr:" +
> hdr.getType()
> + ", error: " + rc.err + ", path: " + rc.path);
> }
> ......
> }
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
