Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Multi threat libs #263
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
base: master
Are you sure you want to change the base?
Multi threat libs #263
Changes from all commits
e0b5c1a
7be52ea
370ccb6
2ec73a6
544fa6f
6da8601
b546efb
9dc0f1f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you adding
--colormap
in this PR also?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be there already, it is an old one. It appears on the currently checked code on the master branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh this is the readme! let me fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do extra work on line 791 to load the default when you can just do it here? You will always reach this line at least once, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't remember the details but there is a timing issue there. Something around things being used before loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the output a string and not a list of strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's what varStrings with only one value put out...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the immortal words of just about everybody..."works for me"!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I had a quick look and the issue is that the default
to_serializable
is used.pytm/pytm/pytm.py
Lines 2015 to 2018 in 9dc0f1f
This is just the same as
The issue is created by
pytm/pytm/pytm.py
Line 2023 in 9dc0f1f
Because of this check for
not nested
inserialize()
.pytm/pytm/pytm.py
Line 2064 in 9dc0f1f
pytm/pytm/pytm.py
Lines 2035 to 2070 in 9dc0f1f
This whole serialize function is a bit overloaded with special handling of member variables and might require a rewrite.
All the checks seem to be for specific classes which are all handle in the same function.
Also why is there the check for
nested
?This is basically equivalent to checking if the class is
TM
.A quick fix would be something like
if instance(obj, TM) and i == "threatsFile"
but oh boy that is not nice.Should this be a new issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds like something to be addressed. @nineinchnick , you there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There appears to be only 1 check for nested. If nested is true, the behavior seems to be potentially undefined (if the code reaches the
not nested
elif).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I started to rewrite the code see #268, but it is difficult to understand what the intention here was.
It seems that the default is
result[i.lstrip("_")] = value
and the cases above only change the value ofvalue
.So setting
nested = True
means that value will not be touched, except it is aElement
,Data
, orThreat
. Or the name of the member isin ("levels", "sourceFiles", "assumptions")
.The last one seems to be a fix for a specific class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a test for when the default is not included in threatsfile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. No. Gotta do one, you're right.