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

dnsdist can be crashed by feeding bad maintenance function in on a console connection #4312

Closed
phonedph1 opened this issue Aug 12, 2016 · 2 comments

Comments

@phonedph1
Copy link
Contributor

As discussed on IRC, feeding in a maintenance function with bad code inside does not get validated until it's actually run on the daemon process which can then cause the daemon to crash. There is no indication of this on the console connection until you try the next operation.

It seems like we should do better than allowing it to crash - either by denying adding anything that can't be fully validated or clearing the maintenance function on errors.

Read configuration from '/etc/dnsdist/dnsdist.conf'

Connecting to 0.0.0.0:5199

> function maintenance() addDynBlocks(exceedNXDOMAINS(1, 10), "Exceeding NXDOMAINs", 60) end



> 

> showServers()

Fatal error: failed in writen2: Broken pipe





> Got control connection from 127.0.0.1:52991

terminate called after throwing an instance of 'LuaContext::ExecutionErrorException'

  what():  [string "function maintenance() addDynBlocks(exceedN..."]:1: attempt to call global 'exceedNXDOMAINS' (a nil value)

Aborted

@phonedph1
Copy link
Contributor Author

phonedph1 commented Aug 12, 2016

13:04 < ahu> ok - well, it might be worth a ticket but I can't imagine how we'd improve this specific usecase
13:04 < ahu> I hadn't even realized you could do this
13:04 < ph1> seems like something people would be doing to add blocks, no?
13:05 < ph1> perhaps I'm just doing something stupid.
13:05 < ahu> yes, but typing in a whole maintenance function on the console isn't what I imagined people would be doing
13:06 < ahu> an issue here is that the console does not get that error message
13:07 < ahu> since it arrives asynchronously
13:08 < ph1> Less concerned about an error on the console vs actually breaking the daemon process by feeding it unvalidated code.
13:08 < ahu> well, I could make it impossible to do this
13:08 < ahu> which is one way to fix it
13:08 < ahu> since we can't validate it usefully
13:09 < ahu> we could stop ourselves from crashing on an error in maintenance() and log it
13:09 < ahu> but that might still make people unhappy
13:09 < ahu> since it fails silently (from the console perspective)
13:10 < ahu> ph1: don't get me wrong, I'd love to be helpful
13:10 < ph1> FWIW even with something incorrect in the config on startup it starts running and then appears to break at the first maintenance call
13:11 < ahu> yes
13:11 < ahu> we might be better off immediately calling maintenance()
13:11 < ahu> once
13:11 < ahu> just so you immediately know it is wrong
13:11 < ahu> instead of 1 second later
13:19 < ph1> I'll drop a issue in and you guys can think about it. I definitely understand both sides (dont put untested code in) but it seems like we can do better than bailing out 
             completely
13:21 < ahu> yes
13:21 < ahu> ticket would be welcome
13:21 < ahu> put a lot of words in there so we remember the discussion when we look at it later
13:22 < ahu> you can also just paste in the irc conversation
13:22 < ahu> would work

@rgacogne
Copy link
Member

rgacogne commented Feb 2, 2024

As far as I can tell this was fixed by #4640

@rgacogne rgacogne closed this as completed Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants