You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "Adam Hunyadi (Jira)" <ji...@apache.org> on 2021/02/17 14:56:00 UTC
[jira] [Updated] (MINIFICPP-1501) Enforce that Processor::onTrigger
calls are always invoked with a valid session
[ https://issues.apache.org/jira/browse/MINIFICPP-1501?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Adam Hunyadi updated MINIFICPP-1501:
------------------------------------
Description:
*Background:*
Processor::onTrigger(..., core::ProcessSession*) calls take sessions by pointers even though they are never expected to be called with a nullptr argument. We could check whether this ptr is null in the onTrigger calls, and use it dereferenced, but this would produce a lot of redundant code. Changing the onTrigger signature would be a serious API break, so we should not change it.
Currently session is not checked for validity before derefencing:
{code:bash|title=Session dereferencing}
➜ git --no-pager grep 'session->' | grep -i 'processors/' | cut -d ":" -f1 | less | sort | uniq | xargs -n1 sh -c 'echo $1 && egrep "if (.*session.*)" $1' sh
extensions/aws/processors/DeleteS3Object.cpp
extensions/aws/processors/FetchS3Object.cpp
extensions/aws/processors/PutS3Object.cpp
extensions/civetweb/processors/ListenHTTP.cpp
extensions/http-curl/processors/InvokeHTTP.cpp
extensions/mqtt/processors/ConsumeMQTT.cpp
extensions/mqtt/processors/ConvertHeartBeat.cpp
extensions/mqtt/processors/ConvertJSONAck.cpp
extensions/mqtt/processors/PublishMQTT.cpp
extensions/openwsman/processors/SourceInitiatedSubscriptionListener.cpp
extensions/sftp/processors/FetchSFTP.cpp
extensions/sftp/processors/ListSFTP.cpp
if (createAndTransferFlowFileFromChild(session, hostname, port, username, file)) {
if (!createAndTransferFlowFileFromChild(session, hostname, port, username, updated_entity)) {
extensions/sftp/processors/PutSFTP.cpp
if (!this->processOne(context, session)) {
extensions/standard-processors/processors/AppendHostInfo.cpp
extensions/standard-processors/processors/ExecuteProcess.cpp
extensions/standard-processors/processors/ExtractText.cpp
extensions/standard-processors/processors/GenerateFlowFile.cpp
extensions/standard-processors/processors/GetFile.cpp
extensions/standard-processors/processors/GetTCP.cpp
extensions/standard-processors/processors/HashContent.cpp
extensions/standard-processors/processors/ListenSyslog.cpp
extensions/standard-processors/processors/LogAttribute.cpp
extensions/standard-processors/processors/PutFile.cpp
extensions/standard-processors/processors/RetryFlowFile.cpp
extensions/standard-processors/processors/RouteOnAttribute.cpp
extensions/standard-processors/processors/TailFile.cpp
if (!session->existsFlowFileInRelationship(Success)) {
extensions/standard-processors/processors/UpdateAttribute.cpp
extensions/standard-processors/tests/unit/GetTCPTests.cpp
extensions/standard-processors/tests/unit/ProcessorTests.cpp
{code}
*Proposal:*
If we want to have this added safety of checking pointers before deferencing them, we can enforce adding a gsl_Expects(session) to all of our implementations that overload the mentioned signature by adding a static code-check next to the linter check that enforces the presence at the start of the function using this overload. We can also provide another overload that takes session by reference - and by default the original implementation would do the checks and call into this one, so overloads on this would not require additional checks. This would mean checking for nullptr in Processor::onTrigger(shrd_ptr<>, shrd_ptr<>) and Processor::onTrigger(*, *) and forwarding the call to Processor::onTrigger(&, &) and moving overloads to the (&, &) version whenever possible.
was:
*Background:*
Processor::onTrigger(..., core::ProcessSession*) calls take sessions by pointers even though they are never expected to be called with a nullptr argument. We could check whether this ptr is null in the onTrigger calls, and use it dereferenced, but this would produce a lot of redundant code. Changing the onTrigger signature would be a serious API break, so we should not change it.
Currently session is not checked for validity before derefencing:
{code:bash|title=Session dereferencing}
➜ git --no-pager grep 'session->' | grep -i 'processors/' | cut -d ":" -f1 | less | sort | uniq | xargs -n1 sh -c 'echo $1 && egrep "if (.*session.*)" $1' sh extensions/aws/processors/DeleteS3Object.cpp
extensions/aws/processors/FetchS3Object.cpp
extensions/aws/processors/PutS3Object.cpp
extensions/civetweb/processors/ListenHTTP.cpp
extensions/http-curl/processors/InvokeHTTP.cpp
extensions/mqtt/processors/ConsumeMQTT.cpp
extensions/mqtt/processors/ConvertHeartBeat.cpp
extensions/mqtt/processors/ConvertJSONAck.cpp
extensions/mqtt/processors/PublishMQTT.cpp
extensions/openwsman/processors/SourceInitiatedSubscriptionListener.cpp
extensions/sftp/processors/FetchSFTP.cpp
extensions/sftp/processors/ListSFTP.cpp
if (createAndTransferFlowFileFromChild(session, hostname, port, username, file)) {
if (!createAndTransferFlowFileFromChild(session, hostname, port, username, updated_entity)) {
extensions/sftp/processors/PutSFTP.cpp
if (!this->processOne(context, session)) {
extensions/standard-processors/processors/AppendHostInfo.cpp
extensions/standard-processors/processors/ExecuteProcess.cpp
extensions/standard-processors/processors/ExtractText.cpp
extensions/standard-processors/processors/GenerateFlowFile.cpp
extensions/standard-processors/processors/GetFile.cpp
extensions/standard-processors/processors/GetTCP.cpp
extensions/standard-processors/processors/HashContent.cpp
extensions/standard-processors/processors/ListenSyslog.cpp
extensions/standard-processors/processors/LogAttribute.cpp
extensions/standard-processors/processors/PutFile.cpp
extensions/standard-processors/processors/RetryFlowFile.cpp
extensions/standard-processors/processors/RouteOnAttribute.cpp
extensions/standard-processors/processors/TailFile.cpp
if (!session->existsFlowFileInRelationship(Success)) {
extensions/standard-processors/processors/UpdateAttribute.cpp
extensions/standard-processors/tests/unit/GetTCPTests.cpp
extensions/standard-processors/tests/unit/ProcessorTests.cpp
{code}
*Proposal:*
If we want to have this added safety of checking pointers before deferencing them, we can enforce adding a gsl_Expects(session) to all of our implementations that overload the mentioned signature by adding a static code-check next to the linter check that enforces the presence at the start of the function using this overload. We can also provide another overload that takes session by reference - and by default the original implementation would do the checks and call into this one, so overloads on this would not require additional checks. This would mean checking for nullptr in Processor::onTrigger(shrd_ptr<>, shrd_ptr<>) and Processor::onTrigger(*, *) and forwarding the call to Processor::onTrigger(&, &) and moving overloads to the (&, &) version whenever possible.
> Enforce that Processor::onTrigger calls are always invoked with a valid session
> -------------------------------------------------------------------------------
>
> Key: MINIFICPP-1501
> URL: https://issues.apache.org/jira/browse/MINIFICPP-1501
> Project: Apache NiFi MiNiFi C++
> Issue Type: Task
> Affects Versions: 0.7.0
> Reporter: Adam Hunyadi
> Assignee: Adam Hunyadi
> Priority: Minor
> Labels: MiNiFi-CPP-Hygiene
> Fix For: 1.0.0
>
>
> *Background:*
> Processor::onTrigger(..., core::ProcessSession*) calls take sessions by pointers even though they are never expected to be called with a nullptr argument. We could check whether this ptr is null in the onTrigger calls, and use it dereferenced, but this would produce a lot of redundant code. Changing the onTrigger signature would be a serious API break, so we should not change it.
> Currently session is not checked for validity before derefencing:
> {code:bash|title=Session dereferencing}
> ➜ git --no-pager grep 'session->' | grep -i 'processors/' | cut -d ":" -f1 | less | sort | uniq | xargs -n1 sh -c 'echo $1 && egrep "if (.*session.*)" $1' sh
> extensions/aws/processors/DeleteS3Object.cpp
> extensions/aws/processors/FetchS3Object.cpp
> extensions/aws/processors/PutS3Object.cpp
> extensions/civetweb/processors/ListenHTTP.cpp
> extensions/http-curl/processors/InvokeHTTP.cpp
> extensions/mqtt/processors/ConsumeMQTT.cpp
> extensions/mqtt/processors/ConvertHeartBeat.cpp
> extensions/mqtt/processors/ConvertJSONAck.cpp
> extensions/mqtt/processors/PublishMQTT.cpp
> extensions/openwsman/processors/SourceInitiatedSubscriptionListener.cpp
> extensions/sftp/processors/FetchSFTP.cpp
> extensions/sftp/processors/ListSFTP.cpp
> if (createAndTransferFlowFileFromChild(session, hostname, port, username, file)) {
> if (!createAndTransferFlowFileFromChild(session, hostname, port, username, updated_entity)) {
> extensions/sftp/processors/PutSFTP.cpp
> if (!this->processOne(context, session)) {
> extensions/standard-processors/processors/AppendHostInfo.cpp
> extensions/standard-processors/processors/ExecuteProcess.cpp
> extensions/standard-processors/processors/ExtractText.cpp
> extensions/standard-processors/processors/GenerateFlowFile.cpp
> extensions/standard-processors/processors/GetFile.cpp
> extensions/standard-processors/processors/GetTCP.cpp
> extensions/standard-processors/processors/HashContent.cpp
> extensions/standard-processors/processors/ListenSyslog.cpp
> extensions/standard-processors/processors/LogAttribute.cpp
> extensions/standard-processors/processors/PutFile.cpp
> extensions/standard-processors/processors/RetryFlowFile.cpp
> extensions/standard-processors/processors/RouteOnAttribute.cpp
> extensions/standard-processors/processors/TailFile.cpp
> if (!session->existsFlowFileInRelationship(Success)) {
> extensions/standard-processors/processors/UpdateAttribute.cpp
> extensions/standard-processors/tests/unit/GetTCPTests.cpp
> extensions/standard-processors/tests/unit/ProcessorTests.cpp
> {code}
> *Proposal:*
> If we want to have this added safety of checking pointers before deferencing them, we can enforce adding a gsl_Expects(session) to all of our implementations that overload the mentioned signature by adding a static code-check next to the linter check that enforces the presence at the start of the function using this overload. We can also provide another overload that takes session by reference - and by default the original implementation would do the checks and call into this one, so overloads on this would not require additional checks. This would mean checking for nullptr in Processor::onTrigger(shrd_ptr<>, shrd_ptr<>) and Processor::onTrigger(*, *) and forwarding the call to Processor::onTrigger(&, &) and moving overloads to the (&, &) version whenever possible.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)