You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Jim Apple (Code Review)" <ge...@cloudera.org> on 2018/10/28 00:16:05 UTC

[Impala-ASF-CR] IMPALA-5031: memcpy cannot take null arguments 

Jim Apple has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11812


Change subject: IMPALA-5031: memcpy cannot take null arguments
......................................................................

IMPALA-5031: memcpy cannot take null arguments

This patch fixes UBSAN "null pointer passed as argument" errors in
data loading. These are undefined behavior according to "7.1.4 Use of
library functions" in the C99 standard (which is included in C++14 in
section [intro.refs]):

    If an argument to a function has an invalid value (such as a value
    outside the domain of the function, or a pointer outside the
    address space of the program, or a null pointer, or a pointer to
    non-modifiable storage when the corresponding parameter is not
    const-qualified) or a type (after promotion) not expected by a
    function with variable number of arguments, the behavior is
    undefined.

The interesting parts of the backtraces for the errors fixed in this
patch are below:

    runtime/string-buffer.h:54:12: runtime error: null pointer passed as argument 1, which is declared to never be null
    /usr/include/string.h:43:45: note: nonnull attribute specified here
    StringBuffer::Append(char const*, long) runtime/string-buffer.h:54:5
    ColumnStatsBase::CopyToBuffer(StringBuffer*, StringValue*) exec/parquet-column-stats.cc:151:51
    ColumnStats<StringValue>::MaterializeStringValuesToInternalBuffers() exec/parquet-column-stats.inline.h:237:70
    HdfsParquetTableWriter::BaseColumnWriter::MaterializeStatsValues() exec/hdfs-parquet-table-writer.cc:149:63
    HdfsParquetTableWriter::AppendRows(RowBatch*, vector<int> const&, bool*) exec/hdfs-parquet-table-writer.cc:1129:53
    HdfsTableSink::WriteRowsToPartition(RuntimeState*, RowBatch*, pair<unique_ptr<OutputPartition, default_delete<OutputPartition> >, vector<int, all> > >*) exec/hdfs-table-sink.cc:256:71
    HdfsTableSink::Send(RuntimeState*, RowBatch*) exec/hdfs-table-sink.cc:591:45

    util/streaming-sampler.h:111:22: runtime error: null pointer passed as argument 2, which is declared to never be null
    /usr/include/string.h:43:45: note: nonnull attribute specified here
    StreamingSampler<long, 64>::SetSamples(int, vector<long> const&) util/streaming-sampler.h:111:5
    RuntimeProfile::Update(vector<TRuntimeProfileNode> const&, int*) util/runtime-profile.cc:313:30
    RuntimeProfile::Update(TRuntimeProfileTree const&) util/runtime-profile.cc:246:3
    Coordinator::BackendState::InstanceStats::Update(TFragmentInstanceExecStatus const&, Coordinator::ExecSummary*, ProgressUpdater*) runtime/coordinator-backend-state.cc:474:13
    Coordinator::BackendState::ApplyExecStatusReport(TReportExecStatusParams const&, Coordinator::ExecSummary*, ProgressUpdater*) runtime/coordinator-backend-state.cc:287:21
    Coordinator::UpdateBackendExecStatus(TReportExecStatusParams const&) runtime/coordinator.cc:679:22
    ClientRequestState::UpdateBackendExecStatus(TReportExecStatusParams const&) service/client-request-state.cc:1254:18
    ImpalaServer::ReportExecStatus(TReportExecStatusResult&, TReportExecStatusParams const&) service/impala-server.cc:1343:18
    ImpalaInternalService::ReportExecStatus(TReportExecStatusResult&, TReportExecStatusParams const&) service/impala-internal-service.cc:87:19

Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51
---
M be/src/runtime/string-buffer.h
M be/src/util/streaming-sampler.h
2 files changed, 3 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/12/11812/1
-- 
To view, visit http://gerrit.cloudera.org:8080/11812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51
Gerrit-Change-Number: 11812
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>

[Impala-ASF-CR] IMPALA-5031: memcpy cannot take null arguments

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11812 )

Change subject: IMPALA-5031: memcpy cannot take null arguments
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/1191/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/11812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51
Gerrit-Change-Number: 11812
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sun, 28 Oct 2018 00:50:17 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: memcpy cannot take null arguments

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11812 )

Change subject: IMPALA-5031: memcpy cannot take null arguments
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11812/1/be/src/runtime/string-buffer.h
File be/src/runtime/string-buffer.h:

http://gerrit.cloudera.org:8080/#/c/11812/1/be/src/runtime/string-buffer.h@93
PS1, Line 93: memcpy
> I see new_buffer is guarded here. Is this one not replaced bc buffer_ is as
UBSAN is a dynamic analysis - it instruments only the places where it actually catches your program violating the rules. In this case, since the line can only get executed when new_buffer is non-null and len_ > 0, this would already segfault if buffer_ were null, so the line never gets tickled, so UBSAN never catches an error.



-- 
To view, visit http://gerrit.cloudera.org:8080/11812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51
Gerrit-Change-Number: 11812
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Nov 2018 13:31:10 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5031: memcpy cannot take null arguments

Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11812 )

Change subject: IMPALA-5031: memcpy cannot take null arguments
......................................................................


Patch Set 1: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/11812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51
Gerrit-Change-Number: 11812
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Nov 2018 17:45:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: memcpy cannot take null arguments

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11812 )

Change subject: IMPALA-5031: memcpy cannot take null arguments
......................................................................

IMPALA-5031: memcpy cannot take null arguments

This patch fixes UBSAN "null pointer passed as argument" errors in
data loading. These are undefined behavior according to "7.1.4 Use of
library functions" in the C99 standard (which is included in C++14 in
section [intro.refs]):

    If an argument to a function has an invalid value (such as a value
    outside the domain of the function, or a pointer outside the
    address space of the program, or a null pointer, or a pointer to
    non-modifiable storage when the corresponding parameter is not
    const-qualified) or a type (after promotion) not expected by a
    function with variable number of arguments, the behavior is
    undefined.

The interesting parts of the backtraces for the errors fixed in this
patch are below:

    runtime/string-buffer.h:54:12: runtime error: null pointer passed as argument 1, which is declared to never be null
    /usr/include/string.h:43:45: note: nonnull attribute specified here
    StringBuffer::Append(char const*, long) runtime/string-buffer.h:54:5
    ColumnStatsBase::CopyToBuffer(StringBuffer*, StringValue*) exec/parquet-column-stats.cc:151:51
    ColumnStats<StringValue>::MaterializeStringValuesToInternalBuffers() exec/parquet-column-stats.inline.h:237:70
    HdfsParquetTableWriter::BaseColumnWriter::MaterializeStatsValues() exec/hdfs-parquet-table-writer.cc:149:63
    HdfsParquetTableWriter::AppendRows(RowBatch*, vector<int> const&, bool*) exec/hdfs-parquet-table-writer.cc:1129:53
    HdfsTableSink::WriteRowsToPartition(RuntimeState*, RowBatch*, pair<unique_ptr<OutputPartition, default_delete<OutputPartition> >, vector<int, all> > >*) exec/hdfs-table-sink.cc:256:71
    HdfsTableSink::Send(RuntimeState*, RowBatch*) exec/hdfs-table-sink.cc:591:45

    util/streaming-sampler.h:111:22: runtime error: null pointer passed as argument 2, which is declared to never be null
    /usr/include/string.h:43:45: note: nonnull attribute specified here
    StreamingSampler<long, 64>::SetSamples(int, vector<long> const&) util/streaming-sampler.h:111:5
    RuntimeProfile::Update(vector<TRuntimeProfileNode> const&, int*) util/runtime-profile.cc:313:30
    RuntimeProfile::Update(TRuntimeProfileTree const&) util/runtime-profile.cc:246:3
    Coordinator::BackendState::InstanceStats::Update(TFragmentInstanceExecStatus const&, Coordinator::ExecSummary*, ProgressUpdater*) runtime/coordinator-backend-state.cc:474:13
    Coordinator::BackendState::ApplyExecStatusReport(TReportExecStatusParams const&, Coordinator::ExecSummary*, ProgressUpdater*) runtime/coordinator-backend-state.cc:287:21
    Coordinator::UpdateBackendExecStatus(TReportExecStatusParams const&) runtime/coordinator.cc:679:22
    ClientRequestState::UpdateBackendExecStatus(TReportExecStatusParams const&) service/client-request-state.cc:1254:18
    ImpalaServer::ReportExecStatus(TReportExecStatusResult&, TReportExecStatusParams const&) service/impala-server.cc:1343:18
    ImpalaInternalService::ReportExecStatus(TReportExecStatusResult&, TReportExecStatusParams const&) service/impala-internal-service.cc:87:19

Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51
Reviewed-on: http://gerrit.cloudera.org:8080/11812
Reviewed-by: Jim Apple <jb...@apache.org>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/string-buffer.h
M be/src/util/streaming-sampler.h
2 files changed, 3 insertions(+), 2 deletions(-)

Approvals:
  Jim Apple: Looks good to me, approved
  Impala Public Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/11812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51
Gerrit-Change-Number: 11812
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>

[Impala-ASF-CR] IMPALA-5031: memcpy cannot take null arguments

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11812 )

Change subject: IMPALA-5031: memcpy cannot take null arguments
......................................................................


Patch Set 2:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3418/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/11812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51
Gerrit-Change-Number: 11812
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Nov 2018 18:20:15 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: memcpy cannot take null arguments

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11812 )

Change subject: IMPALA-5031: memcpy cannot take null arguments
......................................................................


Patch Set 2: Code-Review+2

Carry Vuk's


-- 
To view, visit http://gerrit.cloudera.org:8080/11812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51
Gerrit-Change-Number: 11812
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Nov 2018 18:19:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: memcpy cannot take null arguments

Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11812 )

Change subject: IMPALA-5031: memcpy cannot take null arguments
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11812/1/be/src/runtime/string-buffer.h
File be/src/runtime/string-buffer.h:

http://gerrit.cloudera.org:8080/#/c/11812/1/be/src/runtime/string-buffer.h@93
PS1, Line 93: memcpy
I see new_buffer is guarded here. Is this one not replaced bc buffer_ is assumed to not be null here? Haven't looked too closely,  but it looks like buffer_ could be null.



-- 
To view, visit http://gerrit.cloudera.org:8080/11812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51
Gerrit-Change-Number: 11812
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 29 Oct 2018 18:57:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5031: memcpy cannot take null arguments

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11812 )

Change subject: IMPALA-5031: memcpy cannot take null arguments
......................................................................


Patch Set 2: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/11812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9acc8c32409e67253a987eb3d1fd7d921efcb51
Gerrit-Change-Number: 11812
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Sat, 03 Nov 2018 22:25:28 +0000
Gerrit-HasComments: No