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

Limit recursive struct scan level #60

Closed
wants to merge 1 commit into from

Conversation

ipsusila
Copy link

@ipsusila ipsusila commented Aug 2, 2021

An attempt to fix #57.

Copy link
Owner

@georgysavva georgysavva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for deciding to tackle this!
See comments.

Comment on lines +25 to +40
visitedTypes := make(map[reflect.Type]int)
for len(queue) > 0 {
traversal := queue[0]
queue = queue[1:]
structType := traversal.Type

// remember visited types and number of visits.
if structType.Kind() == reflect.Struct {
if visitCount, visited := visitedTypes[structType]; !visited {
visitedTypes[structType] = 1
} else if visitCount < MaxStructRecursionLevel {
visitedTypes[structType] = visitCount + 1
} else {
continue
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The visitedTypes variable is global for the whole traversal, it counts all type encounters even is separate branches. We should judge recursion only on types that were visited on the path to the current type, i.e. only the current branch of the traversal.
I think we should add visitedTypes as a field to the toTraverse struct, so would every type we iterate over had it's own set of previously visited types.

Comment on lines +75 to 77
for childType.Kind() == reflect.Ptr {
childType = childType.Elem()
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.
I found two more places there we should do the same thing:

if field.Kind() == reflect.Ptr && field.Type().Elem().Kind() == reflect.Struct && field.IsNil() {

scany/dbscan/dbscan.go

Lines 136 to 142 in d03fac1

if elementBaseType.Kind() == reflect.Ptr {
elementBaseTypeElem := elementBaseType.Elem()
if elementBaseTypeElem.Kind() == reflect.Struct {
elementBaseType = elementBaseTypeElem
elementByPtr = true
}
}

I think we should move that change in a separate PR and cover it with tests properly

@georgysavva
Copy link
Owner

Closing due to inactvity. Feel free to reopen.

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.

Scanning into a recursive struct
2 participants