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

Gotcha for &ordered attribute on record fields #179

Open
awelzel opened this issue Feb 17, 2023 · 2 comments
Open

Gotcha for &ordered attribute on record fields #179

awelzel opened this issue Feb 17, 2023 · 2 comments

Comments

@awelzel
Copy link
Contributor

awelzel commented Feb 17, 2023

Talking with @bbannier a bit around, copy(), attributes, and gotchas. The following is a valid script today:

type R: record {
        s: set[string] &ordered &default=set("1", "2", "3", "4", "5");
};

event zeek_init() {
        print R();
}

And it probably doesn't do what was intended:

$ zeek x.zeek
[s={
4,
3,
1,
2,
5
}]

Putting &ordered as part of (or just behind) &default does likely what was intended:

type R: record {
        s: set[string] &default=set("1", "2", "3", "4", "5") &ordered;
};

We may consider this just a variation of the existing gotchas around attributes and how they bind to values rather than vars/fields. And possibly default initialization for containers isn't prevalent enough (though minimally Files::Info has analyzers with &default=string_set() - possibly historically), but maybe warning/erring on such constructs would not a bad idea.

@bbannier
Copy link
Contributor

I think it is currently impossible to use parentheses to group such attributes to make intended semantics clearer. Maybe that would be a first step towards making this easier to control.

@awelzel awelzel transferred this issue from zeek/zeek Apr 5, 2023
@awelzel
Copy link
Contributor Author

awelzel commented Apr 5, 2023

I've moved this to zeek-docs, as documenting might be the most sensible action for now.

awelzel added a commit to zeek/zeek that referenced this issue Dec 15, 2023
The SMB::State$recent_files field is meant to have expiring entries.
However, due to usage of &default=string_set(), the &read_expire
attribute is not respected causing unbounded state growth. Simply remove
&default altogether.

Thanks to ya-sato on Slack for reporting!

Likely related: zeek/zeek-docs#179
awelzel added a commit to zeek/zeek that referenced this issue Dec 15, 2023
The SMB::State$recent_files field is meant to have expiring entries.
However, due to usage of &default=string_set(), the &read_expire
attribute is not respected causing unbounded state growth. Simply remove
&default altogether.

Thanks to ya-sato on Slack for reporting!

Likely related: zeek/zeek-docs#179
awelzel added a commit to zeek/zeek that referenced this issue Dec 15, 2023
The SMB::State$recent_files field is meant to have expiring entries.
However, due to usage of &default=string_set(), the &read_expire
attribute is not respected causing unbounded state growth. Simply remove
&default altogether.

Thanks to ya-sato on Slack for reporting!

Likely related: zeek/zeek-docs#179
awelzel added a commit to zeek/zeek that referenced this issue Dec 17, 2023
The SMB::State$recent_files field is meant to have expiring entries.
However, due to usage of &default=string_set(), the &read_expire
attribute is not respected causing unbounded state growth. Simply remove
&default altogether.

Thanks to ya-sato on Slack for reporting!

Likely related: zeek/zeek-docs#179
awelzel added a commit to zeek/zeek that referenced this issue Dec 17, 2023
The SMB::State$recent_files field is meant to have expiring entries.
However, due to usage of &default=string_set(), the &read_expire
attribute is not respected causing unbounded state growth. Replace
&default=string_set() with &default=set().

Thanks to ya-sato on Slack for reporting!

Related: zeek/zeek-docs#179, #3513.
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

No branches or pull requests

2 participants