Thanks Xiaoqin! Would you be able to open a Jira for this and perhaps submit a PR ? https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute
On Thu, Aug 1, 2019 at 8:23 AM Xiaoqin Fu <[email protected]> wrote: > Dear developers: > I am a Ph.D. student at Washington State University. I applied dynamic > taint analyzer (distTaint) to Apache Zookeeper (version 3.4.11). And then I > find several bugs, that exist from 3.4.11-3.4.14 and 3.5.5, from tainted > paths: > 1. In org.apache.zookeeper.server.ZooKeeperServer: > public ZooKeeperServer(FileTxnSnapLog txnLogFactory, int tickTime, > int minSessionTimeout, int maxSessionTimeout, ZKDatabase zkDb) > { > ...... > LOG.info("Created server with tickTime " + tickTime > + " minSessionTimeout " + getMinSessionTimeout() > + " maxSessionTimeout " + getMaxSessionTimeout() > + " datadir " + txnLogFactory.getDataDir() > + " snapdir " + txnLogFactory.getSnapDir()); > ...... > } > public void finishSessionInit(ServerCnxn cnxn, boolean valid) > ...... > if (!valid) { > LOG.info("Invalid session 0x" > + Long.toHexString(cnxn.getSessionId()) > + " for client " > + cnxn.getRemoteSocketAddress() > + ", probably expired"); > cnxn.sendBuffer(ServerCnxnFactory.closeConn); > } else { > LOG.info("Established session 0x" > + Long.toHexString(cnxn.getSessionId()) > + " with negotiated timeout " + > cnxn.getSessionTimeout() > + " for client " > + cnxn.getRemoteSocketAddress()); > cnxn.enableRecv(); > } > ...... > } > Sensitive information about DataDir, SnapDir, SessionId and > RemoteSocketAddress may be leaked. I think that it is better to add > LOG.isInfoEnabled() conditional statements: > public ZooKeeperServer(FileTxnSnapLog txnLogFactory, int tickTime, > int minSessionTimeout, int maxSessionTimeout, ZKDatabase zkDb) > { > ...... > if (LOG.isInfoEnabled()) > LOG.info("Created server with tickTime " + tickTime > + " minSessionTimeout " + getMinSessionTimeout() > + " maxSessionTimeout " + getMaxSessionTimeout() > + " datadir " + txnLogFactory.getDataDir() > + " snapdir " + txnLogFactory.getSnapDir()); > ...... > } > public void finishSessionInit(ServerCnxn cnxn, boolean valid) { > ...... > if (!valid) { > if (LOG.isInfoEnabled()) > LOG.info("Invalid session 0x" > + Long.toHexString(cnxn.getSessionId()) > + " for client " > + cnxn.getRemoteSocketAddress() > + ", probably expired"); > cnxn.sendBuffer(ServerCnxnFactory.closeConn); > } else { > if (LOG.isInfoEnabled()) > LOG.info("Established session 0x" > + Long.toHexString(cnxn.getSessionId()) > + " with negotiated timeout " + > cnxn.getSessionTimeout() > + " for client " > + cnxn.getRemoteSocketAddress()); > cnxn.enableRecv(); > } > ...... > } > The LOG.isInfoEnabled() conditional statement already exists in > org.apache.zookeeper.server.persistence.FileTxnLog: > public synchronized boolean append(TxnHeader hdr, Record txn) throws > IOException { > { ...... > if(LOG.isInfoEnabled()){ > LOG.info("Creating new log file: " + Util.makeLogName(hdr.getZxid())); > } > ...... > } > > 2. In org.apache.zookeeper.ClientCnxn$SendThread, > void readResponse(ByteBuffer incomingBuffer) throws IOException { > ...... > LOG.warn("Got server path " + event.getPath() > + " which is too short for chroot path " > + chrootPath); > ...... > } > Sensitive information about event path and chroot path may be leaked. The > LOG.isWarnEnabled() conditional statement should be added: > void readResponse(ByteBuffer incomingBuffer) throws IOException { > ...... > if (LOG.isWarnEnabled()) > LOG.warn("Got server path " + event.getPath() > + " which is too short for chroot path " > + chrootPath); > ...... > } > > 3. In org.apache.zookeeper.server.ZooTrace, there are two relevant methods > which all use the same conditional statements: > public static void logTraceMessage(Logger log, long mask, String msg) { > if (isTraceEnabled(log, mask)) { > log.trace(msg); > } > } > > static public void logQuorumPacket(Logger log, long mask, > char direction, QuorumPacket qp) > { > if (isTraceEnabled(log, mask)) { > logTraceMessage(log, mask, direction + > " " + LearnerHandler.packetToString(qp)); > } > } > > If other methods call logQuorumPacket, they execute "if > (isTraceEnabled(log, mask))" conditional statement twice. > We should remove one of two "if (isTraceEnabled(log, mask))" conditional > statements. >
