From 559aeab83648901557f8f00c9c3f206e90da8992 Mon Sep 17 00:00:00 2001
From: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Date: Mon, 20 Mar 2017 15:05:17 -0700
Subject: [PATCH] IMPALA-5088: Fix heap buffer overflow

In a recent patch (IMPALA-4787), we introduced a change where we are
reading past the end of a buffer during a copy. Even though we would
never read the data that was copied from past the end of the buffer,
this could cause a crash if the allocation is sitting on the edge of an
unmapped area of memory. This also caused the ASAN build to fail.

The issue is fixed by never accessing memory that is past the end of the
buffer.

Change-Id: Ia68d69b35d53934e6f1aae2e1696503d17660ca2
Reviewed-on: http://gerrit.cloudera.org:8080/6441
Reviewed-by: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Tested-by: Impala Public Jenkins
---
 be/src/exprs/aggregate-functions-ir.cc | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/be/src/exprs/aggregate-functions-ir.cc b/be/src/exprs/aggregate-functions-ir.cc
index fc4715a0f74..72a78b9b9e7 100644
--- a/be/src/exprs/aggregate-functions-ir.cc
+++ b/be/src/exprs/aggregate-functions-ir.cc
@@ -1066,10 +1066,13 @@ class ReservoirSampleState {
 
     size_t buffer_len = sizeof(ReservoirSampleState<T>) +
         sizeof(ReservoirSample<T>) * num_samples_;
-    StringVal dst = StringVal::CopyFrom(
-        ctx, reinterpret_cast<uint8_t*>(this), buffer_len);
-    memcpy(dst.ptr + sizeof(ReservoirSampleState<T>),
-        reinterpret_cast<uint8_t*>(samples_), sizeof(ReservoirSample<T>) * num_samples_);
+    StringVal dst(ctx, buffer_len);
+    if (LIKELY(!dst.is_null)) {
+      memcpy(dst.ptr, reinterpret_cast<uint8_t*>(this), sizeof(ReservoirSampleState<T>));
+      memcpy(dst.ptr + sizeof(ReservoirSampleState<T>),
+          reinterpret_cast<uint8_t*>(samples_),
+          sizeof(ReservoirSample<T>) * num_samples_);
+    }
     ctx->Free(reinterpret_cast<uint8_t*>(samples_));
     ctx->Free(reinterpret_cast<uint8_t*>(this));
     return dst;