Record fields with &default constructors and expiration attributes may not trigger their timers, causing state leaks

[ckreibich]

As discussed on Slack, in the following snippet from SMB::State$recent_files the &read_expire timer never applies, causing the set to grow indefinitely:

                ## A set of recent files to avoid logging the same
                ## files over and over in the smb files log.
                ## This only applies to files seen in a single connection.
                recent_files : set[string] &default=string_set() &read_expire=3min;

It works when using:

                recent_files : set[string] &default=set() &read_expire=3min;

The cause is differing associativity —

(&default=string_set()) (&read_expire=3min)

vs

&default=(set() &read_expire=3min)

as evidenced by the fact that the following does not parse, since the parser has no alternative to fall back to:

print string_set() &read_expire=3sec;

A few ideas for fixes:

• Of course it'd be ideal if the parser just did the right thing — not sure how feasible that is in the general case because given sufficiently complex attributes this basically becomes arithmetic ... whether you mean 2 + (3 * 2) or (2 + 3) * 2 is up to you.
• Require attributes bound to constructors to be expressed inside parentheses, so &default=(string_set() &read_expire=3min).
• For record fields it doesn’t make sense to have any container attributes associated with them if they’re optional (which &default implies), because those attributes will be lost if the field is initially missing but later populated. So we could check for that case and generate a warning (or error outright).

It might be good to review our attribute code to see whether we have any others that allow expressions that introduce their own attributes.

Scanning the scripts tree finds no other instance where we use &default with an expiration timer and a derived type.

I'm adding this to 6.2 and the backport list for 6.0. Let's see how we resolve this and determine backporting then.

@awelzel, @vpax, fyi.

---

Related: zeek/zeek-docs#179

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

[ckreibich]

#3511 fixes the specific instance of the problem in the SMB code; this ticket is about resolving the broader parsing problem.