HBASE-15946. Eliminate possible security concerns in Store File metrics. Invoking 'hbase hfile' inside a servlet raises several concerns. This patch avoids invoking a separate process, and also adds validation that the file being read is at least inside the HBase root directory.
Signed-off-by: Mikhail Antonov <anto...@apache.org> Project: http://git-wip-us.apache.org/repos/asf/hbase/repo Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/6da6babe Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/6da6babe Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/6da6babe Branch: refs/heads/hbase-12439 Commit: 6da6babe4faa7b2b16775d3cd5c861e71ef4cf31 Parents: babdedc Author: Sean Mackrory <mackror...@apache.org> Authored: Tue May 31 10:28:27 2016 -0600 Committer: Mikhail Antonov <anto...@apache.org> Committed: Thu Jun 9 16:08:19 2016 -0700 ---------------------------------------------------------------------- .../hbase/io/hfile/HFilePrettyPrinter.java | 108 ++++++++++++------- .../hbase-webapps/regionserver/storeFile.jsp | 35 +++--- 2 files changed, 83 insertions(+), 60 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hbase/blob/6da6babe/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePrettyPrinter.java ---------------------------------------------------------------------- diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePrettyPrinter.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePrettyPrinter.java index e9e21fe..36067e5 100644 --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePrettyPrinter.java +++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFilePrettyPrinter.java @@ -1,4 +1,3 @@ - /* * * Licensed to the Apache Software Foundation (ASF) under one @@ -115,6 +114,8 @@ public class HFilePrettyPrinter extends Configured implements Tool { private Map<String, List<Path>> mobFileLocations; private static final int FOUND_MOB_FILES_CACHE_CAPACITY = 50; private static final int MISSING_MOB_FILES_CACHE_CAPACITY = 20; + private PrintStream out = System.out; + private PrintStream err = System.err; /** * The row which the user wants to specify and print all the KeyValues for. @@ -161,6 +162,11 @@ public class HFilePrettyPrinter extends Configured implements Tool { options.addOptionGroup(files); } + public void setPrintStreams(PrintStream out, PrintStream err) { + this.out = out; + this.err = err; + } + public boolean parseOptions(String args[]) throws ParseException, IOException { if (args.length == 0) { @@ -192,7 +198,7 @@ public class HFilePrettyPrinter extends Configured implements Tool { row = Bytes.toBytesBinary(key); isSeekToRow = true; } else { - System.err.println("Invalid row is specified."); + err.println("Invalid row is specified."); System.exit(-1); } } @@ -206,17 +212,17 @@ public class HFilePrettyPrinter extends Configured implements Tool { String enc = HRegionInfo.encodeRegionName(rn); Path regionDir = new Path(tableDir, enc); if (verbose) - System.out.println("region dir -> " + regionDir); + out.println("region dir -> " + regionDir); List<Path> regionFiles = HFile.getStoreFiles(FileSystem.get(getConf()), regionDir); if (verbose) - System.out.println("Number of region files found -> " + out.println("Number of region files found -> " + regionFiles.size()); if (verbose) { int i = 1; for (Path p : regionFiles) { if (verbose) - System.out.println("Found file[" + i++ + "] -> " + p); + out.println("Found file[" + i++ + "] -> " + p); } } files.addAll(regionFiles); @@ -255,27 +261,46 @@ public class HFilePrettyPrinter extends Configured implements Tool { // iterate over all files found for (Path fileName : files) { try { - processFile(fileName); + int exitCode = processFile(fileName); + if (exitCode != 0) { + return exitCode; + } } catch (IOException ex) { LOG.error("Error reading " + fileName, ex); - System.exit(-2); + return -2; } } if (verbose || printKey) { - System.out.println("Scanned kv count -> " + count); + out.println("Scanned kv count -> " + count); } return 0; } - private void processFile(Path file) throws IOException { + public int processFile(Path file) throws IOException { if (verbose) - System.out.println("Scanning -> " + file); + out.println("Scanning -> " + file); + + Path rootPath = FSUtils.getRootDir(getConf()); + String rootString = rootPath + rootPath.SEPARATOR; + if (!file.toString().startsWith(rootString)) { + // First we see if fully-qualified URI matches the root dir. It might + // also be an absolute path in the same filesystem, so we prepend the FS + // of the root dir and see if that fully-qualified URI matches. + FileSystem rootFS = rootPath.getFileSystem(getConf()); + String qualifiedFile = rootFS.getUri().toString() + file.toString(); + if (!qualifiedFile.startsWith(rootString)) { + err.println("ERROR, file (" + file + + ") is not in HBase's root directory (" + rootString + ")"); + return -2; + } + } + FileSystem fs = file.getFileSystem(getConf()); if (!fs.exists(file)) { - System.err.println("ERROR, file doesnt exist: " + file); - System.exit(-2); + err.println("ERROR, file doesnt exist: " + file); + return -2; } HFile.Reader reader = HFile.createReader(fs, file, new CacheConfig(getConf()), getConf()); @@ -306,12 +331,12 @@ public class HFilePrettyPrinter extends Configured implements Tool { } if (printBlockIndex) { - System.out.println("Block Index:"); - System.out.println(reader.getDataBlockIndexReader()); + out.println("Block Index:"); + out.println(reader.getDataBlockIndexReader()); } if (printBlockHeaders) { - System.out.println("Block Headers:"); + out.println("Block Headers:"); /* * TODO: this same/similar block iteration logic is used in HFileBlock#blockRange and * TestLazyDataBlockDecompression. Refactor? @@ -327,16 +352,17 @@ public class HFilePrettyPrinter extends Configured implements Tool { block = reader.readBlock(offset, -1, /* cacheBlock */ false, /* pread */ false, /* isCompaction */ false, /* updateCacheMetrics */ false, null, null); offset += block.getOnDiskSizeWithHeader(); - System.out.println(block); + out.println(block); } } if (printStats) { fileStats.finish(); - System.out.println("Stats:\n" + fileStats); + out.println("Stats:\n" + fileStats); } reader.close(); + return 0; } private void scanKeysValues(Path file, KeyValueStatsCollector fileStats, @@ -361,24 +387,24 @@ public class HFilePrettyPrinter extends Configured implements Tool { } // dump key value if (printKey) { - System.out.print("K: " + cell); + out.print("K: " + cell); if (printValue) { - System.out.print(" V: " + out.print(" V: " + Bytes.toStringBinary(cell.getValueArray(), cell.getValueOffset(), cell.getValueLength())); int i = 0; List<Tag> tags = TagUtil.asList(cell.getTagsArray(), cell.getTagsOffset(), cell.getTagsLength()); for (Tag tag : tags) { - System.out.print(String.format(" T[%d]: %s", i++, tag.toString())); + out.print(String.format(" T[%d]: %s", i++, tag.toString())); } } - System.out.println(); + out.println(); } // check if rows are in order if (checkRow && pCell != null) { if (CellComparator.COMPARATOR.compareRows(pCell, cell) > 0) { - System.err.println("WARNING, previous row is greater then" + err.println("WARNING, previous row is greater then" + " current row\n\tfilename -> " + file + "\n\tprevious -> " + CellUtil.getCellKeyAsString(pCell) + "\n\tcurrent -> " + CellUtil.getCellKeyAsString(cell)); @@ -389,12 +415,12 @@ public class HFilePrettyPrinter extends Configured implements Tool { String fam = Bytes.toString(cell.getFamilyArray(), cell.getFamilyOffset(), cell.getFamilyLength()); if (!file.toString().contains(fam)) { - System.err.println("WARNING, filename does not match kv family," + err.println("WARNING, filename does not match kv family," + "\n\tfilename -> " + file + "\n\tkeyvalue -> " + CellUtil.getCellKeyAsString(cell)); } if (pCell != null && CellComparator.compareFamilies(pCell, cell) != 0) { - System.err.println("WARNING, previous kv has different family" + err.println("WARNING, previous kv has different family" + " compared to current key\n\tfilename -> " + file + "\n\tprevious -> " + CellUtil.getCellKeyAsString(pCell) + "\n\tcurrent -> " + CellUtil.getCellKeyAsString(cell)); @@ -492,32 +518,32 @@ public class HFilePrettyPrinter extends Configured implements Tool { private void printMeta(HFile.Reader reader, Map<byte[], byte[]> fileInfo) throws IOException { - System.out.println("Block index size as per heapsize: " + out.println("Block index size as per heapsize: " + reader.indexSize()); - System.out.println(asSeparateLines(reader.toString())); - System.out.println("Trailer:\n " + out.println(asSeparateLines(reader.toString())); + out.println("Trailer:\n " + asSeparateLines(reader.getTrailer().toString())); - System.out.println("Fileinfo:"); + out.println("Fileinfo:"); for (Map.Entry<byte[], byte[]> e : fileInfo.entrySet()) { - System.out.print(FOUR_SPACES + Bytes.toString(e.getKey()) + " = "); + out.print(FOUR_SPACES + Bytes.toString(e.getKey()) + " = "); if (Bytes.compareTo(e.getKey(), Bytes.toBytes("MAX_SEQ_ID_KEY")) == 0) { long seqid = Bytes.toLong(e.getValue()); - System.out.println(seqid); + out.println(seqid); } else if (Bytes.compareTo(e.getKey(), Bytes.toBytes("TIMERANGE")) == 0) { TimeRangeTracker timeRangeTracker = TimeRangeTracker.getTimeRangeTracker(e.getValue()); - System.out.println(timeRangeTracker.getMin() + "...." + timeRangeTracker.getMax()); + out.println(timeRangeTracker.getMin() + "...." + timeRangeTracker.getMax()); } else if (Bytes.compareTo(e.getKey(), FileInfo.AVG_KEY_LEN) == 0 || Bytes.compareTo(e.getKey(), FileInfo.AVG_VALUE_LEN) == 0) { - System.out.println(Bytes.toInt(e.getValue())); + out.println(Bytes.toInt(e.getValue())); } else { - System.out.println(Bytes.toStringBinary(e.getValue())); + out.println(Bytes.toStringBinary(e.getValue())); } } try { - System.out.println("Mid-key: " + (CellUtil.getCellKeyAsString(reader.midkey()))); + out.println("Mid-key: " + (CellUtil.getCellKeyAsString(reader.midkey()))); } catch (Exception e) { - System.out.println ("Unable to retrieve the midkey"); + out.println ("Unable to retrieve the midkey"); } // Printing general bloom information @@ -526,12 +552,12 @@ public class HFilePrettyPrinter extends Configured implements Tool { if (bloomMeta != null) bloomFilter = BloomFilterFactory.createFromMeta(bloomMeta, reader); - System.out.println("Bloom filter:"); + out.println("Bloom filter:"); if (bloomFilter != null) { - System.out.println(FOUR_SPACES + bloomFilter.toString().replaceAll( + out.println(FOUR_SPACES + bloomFilter.toString().replaceAll( BloomFilterUtil.STATS_RECORD_SEP, "\n" + FOUR_SPACES)); } else { - System.out.println(FOUR_SPACES + "Not present"); + out.println(FOUR_SPACES + "Not present"); } // Printing delete bloom information @@ -540,13 +566,13 @@ public class HFilePrettyPrinter extends Configured implements Tool { if (bloomMeta != null) bloomFilter = BloomFilterFactory.createFromMeta(bloomMeta, reader); - System.out.println("Delete Family Bloom filter:"); + out.println("Delete Family Bloom filter:"); if (bloomFilter != null) { - System.out.println(FOUR_SPACES + out.println(FOUR_SPACES + bloomFilter.toString().replaceAll(BloomFilterUtil.STATS_RECORD_SEP, "\n" + FOUR_SPACES)); } else { - System.out.println(FOUR_SPACES + "Not present"); + out.println(FOUR_SPACES + "Not present"); } } http://git-wip-us.apache.org/repos/asf/hbase/blob/6da6babe/hbase-server/src/main/resources/hbase-webapps/regionserver/storeFile.jsp ---------------------------------------------------------------------- diff --git a/hbase-server/src/main/resources/hbase-webapps/regionserver/storeFile.jsp b/hbase-server/src/main/resources/hbase-webapps/regionserver/storeFile.jsp index cbbb61f..fe8cfe0 100644 --- a/hbase-server/src/main/resources/hbase-webapps/regionserver/storeFile.jsp +++ b/hbase-server/src/main/resources/hbase-webapps/regionserver/storeFile.jsp @@ -18,20 +18,15 @@ */ --%> <%@ page contentType="text/html;charset=UTF-8" - import="java.util.Collection" - import="java.util.Date" - import="java.util.List" import="java.io.ByteArrayOutputStream" import="java.io.PrintStream" - import="java.io.BufferedReader" - import="java.io.InputStreamReader" import="org.apache.hadoop.conf.Configuration" + import="org.apache.hadoop.fs.Path" import="org.apache.hadoop.hbase.HBaseConfiguration" import="org.apache.hadoop.hbase.io.hfile.HFilePrettyPrinter" import="org.apache.hadoop.hbase.regionserver.HRegionServer" - import="org.apache.hadoop.hbase.regionserver.Region" - import="org.apache.hadoop.hbase.regionserver.Store" - import="org.apache.hadoop.hbase.regionserver.StoreFile"%> + import="org.apache.hadoop.hbase.regionserver.StoreFile" + %> <% String storeFile = request.getParameter("name"); HRegionServer rs = (HRegionServer) getServletContext().getAttribute(HRegionServer.REGIONSERVER); @@ -91,17 +86,19 @@ <pre> <% try { - ProcessBuilder pb=new ProcessBuilder("hbase", "hfile", "-s", "-f", storeFile); - pb.redirectErrorStream(true); - Process pr = pb.start(); - BufferedReader in = new BufferedReader(new InputStreamReader(pr.getInputStream())); - String line; - while ((line = in.readLine()) != null) {%> - <%= line %> - <%} - pr.waitFor(); - in.close(); - } + ByteArrayOutputStream byteStream = new ByteArrayOutputStream(); + PrintStream printerOutput = new PrintStream(byteStream); + HFilePrettyPrinter printer = new HFilePrettyPrinter(); + printer.setPrintStreams(printerOutput, printerOutput); + printer.setConf(conf); + String[] options = {"-s"}; + printer.parseOptions(options); + printer.processFile(new Path(storeFile)); + String text = byteStream.toString();%> + <%= + text + %> + <%} catch (Exception e) {%> <%= e %> <%}