Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dnn: Fix dangling pointers returned by GetLayerNames #927

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

deradiri
Copy link
Contributor

@deradiri deradiri commented Jan 25, 2022

GetLayerNames returns an array of char pointers to cstrings in a vector<string>; unfortunately, once the vector is out of scope, the strings are destroyed. GetLayerNames callers are then left with dangling pointers.

This change fixes the problem by expanding the strs buffer returned by GetLayerNames and copying the vector's cstrings into it.

Testing:
See this comment

Here's an example of the bug in action:

	n := net.GetLayerNames()

	for i, l := range n {
		fmt.Printf("Layer %d: %s\n", i, l)
	}

Before fix:

$ go test -tags customenv,static
Layer 0: @�
Layer 1: 0�
...

After fix:

$ go test -tags customenv,static
Layer 0: detector/yolo-v3-tiny/Conv_12/BiasAdd/YoloRegion
Layer 1: detector/yolo-v3-tiny/Conv_9/BiasAdd/YoloRegion
...

@deadprogram
Copy link
Member

Yes, I see how the previous implementation was lacking. I just wonder however how this test here has been passing?

if len(lnames) == 142 && lnames[1] != "conv1/relu_7x7" {

@deradiri
Copy link
Contributor Author

@deadprogram Good question... I guess maybe it's that the the caffe backend keeps the layer names around for longer/permanently?

I discovered this issue using openvino, will see if I can add a test case. Is there some procedure for adding models as test artifacts?

@deradiri deradiri force-pushed the fix-getlayernames-dangling-pointers branch from 5bde20d to 891e7a4 Compare July 15, 2022 12:22
'GetLayerNames' returns an array of 'char' pointers to cstrings
in a 'vector<string>'; unfortunately, once the vector is out of
scope, the strings are destroyed. 'GetLayerNames' callers are then
left with dangling pointers.

This change fixes the problem by expanding the 'strs' buffer
returned by 'GetLayerNames' and copying the vector's cstrings into it.
@deradiri
Copy link
Contributor Author

deradiri commented Jul 21, 2022

Yes, I see how the previous implementation was lacking. I just wonder however how this test here has been passing?

if len(lnames) == 142 && lnames[1] != "conv1/relu_7x7" {

@deadprogram It turns out this test passes by pure luck, I ran it a few times and logged all the layers. Below is a diff between two runs:

image

The layer name at index 1 seems to always be intact, so rather than add a new model I'll update the test to check a set of layers distributed throughout the array. Diff of runs with and without the fix in this PR is below:
image

@deradiri deradiri force-pushed the fix-getlayernames-dangling-pointers branch from 891e7a4 to f776143 Compare July 21, 2022 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants