ARM-software / armnn

Arm NN ML Software. The code here is a read-only mirror of https://review.mlplatform.org/admin/repos/ml/armnn
https://developer.arm.com/products/processors/machine-learning/arm-nn
MIT License
1.17k stars 309 forks source link

Wrong variable 'm_ProjectionWeights' is used to check if the pointer is valid #639

Closed sider-bughunter closed 2 years ago

sider-bughunter commented 2 years ago

I have noticed that the m_ProjectionWeights variable is used to check if the pointer is valid in this code block. To be consistent with other usages such as this code and this, it should be replaced with m_ProjectionBias. Otherwise, this could be a potential cause of segmentation fault at the subsequence line std::make_shared<ScopedTensorHandle>(*(params.m_ProjectionBias)); (correct me if I am wrong)

diff --git a/src/armnn/Network.cpp b/src/armnn/Network.cpp
index 8ec8b42..67fd6df 100644
--- a/src/armnn/Network.cpp
+++ b/src/armnn/Network.cpp
@@ -2598,25 +2598,25 @@ IConnectableLayer* NetworkImpl::AddQLstmLayer(const QLstmDescriptor&  descriptor
     // QLstm Projection parameters
     if(descriptor.m_ProjectionEnabled)
     {
         if(params.m_ProjectionWeights == nullptr)
         {
             throw InvalidArgumentException("AddQLstmLayer: Projection Weights cannot be NULL");
         }

         layer->m_ProjectionParameters.m_ProjectionWeights =
                 std::make_shared<ScopedTensorHandle>(*(params.m_ProjectionWeights));

         // Projection bias is optional even if projection is enabled
-        if(params.m_ProjectionWeights != nullptr)
+        if(params.m_ProjectionBias != nullptr)
         {
             layer->m_ProjectionParameters.m_ProjectionBias =
                     std::make_shared<ScopedTensorHandle>(*(params.m_ProjectionBias));
         }

     }

     // QLstm Peephole params
     if(descriptor.m_PeepholeEnabled)
     {
         if(params.m_CellToForgetWeights == nullptr)
         {
catcor01 commented 2 years ago

Hello, yes you are correct. I have submitted a patch here https://review.mlplatform.org/c/ml/armnn/+/7527. Thank you for reporting this. Cathal.

MikeJKelly commented 2 years ago

https://review.mlplatform.org/c/ml/armnn/+/7527 has been merged, thanks for the report @sider-bughunter !