Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

MKLDNNConvolutionBackward accesses out of bound elements #17992

Closed
leezu opened this issue Apr 7, 2020 · 2 comments · Fixed by #17997
Closed

MKLDNNConvolutionBackward accesses out of bound elements #17992

leezu opened this issue Apr 7, 2020 · 2 comments · Fixed by #17997
Assignees

Comments

@leezu
Copy link
Contributor

leezu commented Apr 7, 2020

Description

CI with updated toolchain (ie #17984) catches the bug.

vector: :_M_range_check: __n (which is 2) >= this->size() (which is 2)

Error Message

To Reproduce

Build with this simple patch

diff --git a/src/operator/nn/mkldnn/mkldnn_convolution.cc b/src/operator/nn/mkldnn/mkldnn_convolution.cc
index ada42a22c..95b44fd92 100644
--- a/src/operator/nn/mkldnn/mkldnn_convolution.cc
+++ b/src/operator/nn/mkldnn/mkldnn_convolution.cc
@@ -480,7 +480,7 @@ void MKLDNNConvolutionBackward(const nnvm::NodeAttrs& attrs, const OpContext &ct
                                            {MKLDNN_ARG_DIFF_SRC, *in_grad_mem.second}});
     CommitOutput(in_grad[conv::kData], in_grad_mem);
   }
-  if (req[conv::kWeight] || req[conv::kBias]) {
+  if (req.at(conv::kWeight) || req.at(conv::kBias)) {
     if (convBwd.GetDataPd().diff_dst_desc() != convBwd.GetWeightsPd().diff_dst_desc())
       out_grad_mem = out_grad.GetMKLDNNDataReorder(convBwd.GetWeightsPd().diff_dst_desc());
     auto data_mem = data.GetMKLDNNDataReorder(convBwd.GetWeightsPd().src_desc());

OR follow the instructions in #17987 to trigger this via glibc assertions in debug build.

Run test_operator.test_convolution_independent_gradients to trigger the bug.

cc @TaoLv

@leezu leezu added the Bug label Apr 7, 2020
@leezu
Copy link
Contributor Author

leezu commented Apr 7, 2020

Notice that there are also other issues with this test
#15631
#15638

@TaoLv TaoLv self-assigned this Apr 8, 2020
@TaoLv TaoLv added the MKLDNN label Apr 8, 2020
@TaoLv
Copy link
Member

TaoLv commented Apr 8, 2020

I doubt whether #15638 is related. It seems it's a correctness issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants