Refactor IPC target validation

[JerziKaminsky]

Followup to #1173

• ipc config option shouldn't fail if target doesn't exist (don't refuse to create an ipc policy just because the file doesn't exist. Borks startup)
• Added resolve_ipc_path() to reduce boilerplate
• Policy allocators now store the result of validate_ipc_target() under policy->validated.
• cmd_permit/_reject use policy->validated to make decisions.
• sway_log instead of sway_assert on unvalidated target, we don't want to abort, even in debug builds.

[ddevault]

ipc config option shouldn't fail if target doesn't exist (don't refuse to create a an ipc just because the file doesn't exist. Borks startup)

Why? In what situation does this occur? Seems to incur extra complexity to handle an unlikely scenario imo.

A lot of changes seem to stem from that. I don't like the idea of pushing validation deeper into runtime, I'd rather do it all on startup - give warnings for bad configurations straight away and simplify checks later on.

[JerziKaminsky]

Why? In what situation does this occur? Seems to incur extra complexity to handle an unlikely scenario imo.

Right now, a policy targeting a missing file prevents sway from starting up:

sudo rm /usr/bin/swaybar
sway
04/20/17 20:17:55 - [permit.c:62] cmd_permit:Unable to resolve IPC permit target '/usr/bin/swaybar'. will issue empty policy
04/20/17 20:17:55 - [security.c:156] Unable to allocate security policy
./test.sh: line 20: 19485 Segmentation fault      (core dumped) sway -V

The policy allocators can currently only communicate with caller by returning a policy or NULL.
On the other hand allocators don't have the necessary context in order to perform policy decisions -- that's the caller's job (warn, don't give perms, ignore). This change puts the result of validation into new newly created policy so the caller can decide what to do.

I think we do want to integrate validation down into the policy allocators, so that it's impossible to "forget" to do.

A lot of changes seem to stem from that.

not really, I just took the opportunity to also tidy up.

[ddevault]

We probably shouldn't abort, but we shouldn't defer validation. We should just ignore the policy and print a warning.

[ddevault]

Policy allocators now store the result of validate_ipc_target() under policy->validated.

cmd_permit/_reject use policy->validated to make decisions.

These changes are ones that make me concerned. By the time you obtain a reference to a policy, that policy should have already been validated.

[JerziKaminsky]

We should just ignore the policy and print a warning.

What exactly do you mean by "ignore"? Earlier you asked that permit return an empty policy. It did and does.

We probably shouldn't abort, but we shouldn't defer validation

We don't.

that policy should have already been validated.

"checked" or "enforced"?

Anyway, right now if validation fails the allocator tells the caller it's out of memory. That's wrong. This PR fixes it. That's it.

[ddevault]

Anyway, right now if validation fails the allocator tells the caller it's out of memory. That's wrong. This PR fixes it. That's it.

Returning NULL doesn't signal OOM. It just means it didn't work. It would set errno = ENOMEM if we wanted to specify the specific failure, but that's not important here.

[ddevault]

I still haven't looked over the code, but the only change I think is necessary is to avoid aborting if the target doesn't exist.

[JerziKaminsky]

Returning NULL doesn't signal OOM. It just means it didn't work.

the only change I think is necessary is to avoid aborting if the target doesn't exist.

How will the caller tell the difference?

[ddevault]

Why does the caller need to tell the difference?

[JerziKaminsky]

Why does the caller need to tell the difference?

Because an OOM is non-recoverable and a bad validation potentially is.

You seem to feel that the change needed to fix this is trivial and what I've done unnecessary, so I'm going to just close this and open an issue instead. You can deal with it as you think best.