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

Enforce the is_node_list contract in lib-config setters. #581

Merged
merged 1 commit into from Dec 8, 2016

Conversation

Projects
None yet
3 participants
@LemonBoy
Member

LemonBoy commented Nov 29, 2016

An assertion failure is better than a segfault.
This complements #570

Enforce the is_node_list contract in lib-config setters.
An assertion failure is better than a segfault.
@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Nov 29, 2016

imo the config file should be fixed up instead and g_critical be thrown, similar to https://github.com/irssi/irssi/blob/master/src/lib-config/set.c#L108
not exactly sure what needs to be taken care of since in this case we need to fix parent and mainnode

@dequis

This comment has been minimized.

Member

dequis commented Nov 29, 2016

imo the config file should be fixed up

How can we fix something that wasn't intended to make sense? This thing is just the result of fuzzing, not actual user mistakes.

@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Nov 29, 2016

@dequis

This comment has been minimized.

Member

dequis commented Nov 29, 2016

Fair enough.

So i tested with this config

chatnets{0 0

And applying both this PR and the other one results in these visible errors:

Error: Ignored errors in configuration file:
Error: /home/dx/test/irssi/irssi/con:1: Warning: missing '='
Error: /home/dx/test/irssi/irssi/con:1: Warning: missing '='
Error: /home/dx/test/irssi/irssi/con:2: Warning: missing ';'
Error: /home/dx/test/irssi/irssi/con:2: error: unexpected end of file, expected character '}'
Error:

And it saves to:

chatnets = { 0 = "0"; };
settings = {
  core = { real_name = "dequis"; user_name = "dx"; nick = "dx"; };
  "fe-text" = { actlist_sort = "refnum"; };
};

Which loads just fine afterwards.

So, merge both?

@dequis

This comment has been minimized.

Member

dequis commented Nov 29, 2016

new commit

Well this is awkward.

No visible change in behavior compared to the configs in my previous comment. A breakpoint in line 102 (the g_critical() one) is never hit. It's possible that this PR might have nothing to do with the thing I tested.

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Nov 29, 2016

The second commit tries to save the date by nuking the parent node and re-creating it in place as a list/block one. I'm not 100% sold on the usefulness of this approach since we're substituting a broken and syntactically invalid configuration with a now syntactically-valid but possibly still broken one.

eg. the code can't do much when the parent key is wrong

I'm using the following snippet as test vector

0 0
chatnets{0""
@ailin-nemui

This comment has been minimized.

Contributor

ailin-nemui commented Nov 30, 2016

it doesn't work properly for some reason (probably a thought error on my side)
starting with

0 0
chatnets{""0

turns into

0 = "0";
chatnets = {  = { type = "IRC"; }; };

on first run and into

0 = "0";
chatnets = { type = { type = "IRC"; }; };

on 2nd

with no empty key (i.e. chatnets{0"") it works more stable, resulting in

0 = "0";
chatnets = { 0 = { type = "IRC"; }; };

on first try. on another topic, on the initial config read, g_critical should stop irssi from starting up so you get a chance to fix your config. but this operation seems to happen after I already lowered the priority and the screen has been initialized, so the warning only blinks for a second before being replaced by the TUI

so maybe we should just revert to your initial suggestion :$

@LemonBoy

This comment has been minimized.

Member

LemonBoy commented Nov 30, 2016

(It does output the 2nd snippet for me, after a load + /quit cycle)
That's my point, it can't work, hence my suggestion to merge the 1st commit /and/ #570

@ailin-nemui ailin-nemui merged commit 618c8bd into irssi:master Dec 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment