zookeeper-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Xiaoqin Fu <xiaoqin...@gmail.com>
Subject Apache Zookeeper Bugs
Date Thu, 01 Aug 2019 02:11:24 GMT
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.

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message