zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Shraer <shra...@gmail.com>
Subject Re: Apache Zookeeper Bugs
Date Thu, 01 Aug 2019 16:29:56 GMT
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 <xiaoqin.fu@gmail.com> 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.
>

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