Skip to content

Try fix MultipleTypeField: Add post_inits metric#1485

Closed
gpotter2 wants to merge 1 commit intosecdev:masterfrom
gpotter2:socks-testfix
Closed

Try fix MultipleTypeField: Add post_inits metric#1485
gpotter2 wants to merge 1 commit intosecdev:masterfrom
gpotter2:socks-testfix

Conversation

@gpotter2
Copy link
Copy Markdown
Member

fixes #1484

Is certainly improvable, any ideas will be appreciated

@gpotter2 gpotter2 mentioned this pull request Jun 21, 2018
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jun 21, 2018

Codecov Report

Merging #1485 into master will increase coverage by 0.07%.
The diff coverage is 97.14%.

@@            Coverage Diff             @@
##           master    #1485      +/-   ##
==========================================
+ Coverage   85.28%   85.36%   +0.07%     
==========================================
  Files         177      177              
  Lines       40855    40879      +24     
==========================================
+ Hits        34845    34896      +51     
+ Misses       6010     5983      -27
Impacted Files Coverage Δ
scapy/packet.py 76.49% <100%> (+0.15%) ⬆️
scapy/asn1fields.py 84.63% <100%> (+0.03%) ⬆️
scapy/layers/dns.py 91.45% <100%> (+0.03%) ⬆️
scapy/fields.py 91.15% <96%> (+0.09%) ⬆️
scapy/arch/windows/__init__.py 78.84% <0%> (+2.62%) ⬆️
scapy/pton_ntop.py 97.26% <0%> (+8.21%) ⬆️

def i2m(self, pkt, x):
if x == b".":
return b"\x00"
elif isinstance(x, str):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure when/if this can happen, as "internal" value should always be bytes.

@p-l-
Copy link
Copy Markdown
Member

p-l- commented Jun 23, 2018

Thanks for this work and your explanations, sorry it took me so long to understand what you wanted to do. Apart from my remark about str objects in DNSStrField internal values, it's all good to me.

@gpotter2
Copy link
Copy Markdown
Member Author

I don't really like myself the extra layer of complexity that this PR adds :/ I really would have like a different fix. If you have time to look into it, it would be great

self.packetfields = []
self.payload = NoPayload()
self.post_inits = []
self.init_fields()
Copy link
Copy Markdown
Member Author

@gpotter2 gpotter2 Jul 21, 2018

Choose a reason for hiding this comment

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

NOTE TO SELF: just order the fields following the fields_desc order during initialization. That will fix the problem way easier. (The arguments will be intianciated correctly. See example in #1484)

Use stg like

for f in fields_desc:
    v = kwargs.get(v, None)
    if v:
        self.setfieldval.....

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

FTR, PR moved to #1550

@gpotter2 gpotter2 closed this Jul 21, 2018
@gpotter2 gpotter2 deleted the socks-testfix branch July 19, 2020 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MultipleTypeField: wrong initialization

3 participants