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

Add runString(s) #2380

Closed
wants to merge 2 commits into from
Closed

Add runString(s) #2380

wants to merge 2 commits into from

Conversation

Vurv78
Copy link
Member

@Vurv78 Vurv78 commented Jul 31, 2022

A better way to do entity():remoteSetCode(...) that doesn't have a cooldown / need to transfer with net.
Resolves some old issues that were closed due to having remoteSetCode as an alternative.

Examples

@persist Test:string

Test = "Hm"

try {
	# Has access to @persist, @outputs, @inputs variables, no scope variables, though.
	runString("error(Test)")
} catch (Err) {
	assert(Err == Test)
}
@strict

for (I = 1, 60) {
	runString("print(" + I + ")")   
}

@Divran
Copy link
Contributor

Divran commented Jul 31, 2022

This shouldn't be placed in an extension which is enabled by default.
Either make it its own file and extension (runstring.lua) or put it in the same file as remoteSetCode

I also recommend putting the majority of this code in a separate function (in e2lib or something) and calling from the runstring function and also from here https://github.com/wiremod/wire/blob/master/lua/entities/gmod_wire_expression2/init.lua#L272-L284 to reduce duplicate code

also the ops cost of this needs to be significantly higher than 500, or have a cooldown of 1 second like remoteSetCode, or I imagine it would be exceptionally easy to crash servers with it. It's probably not that difficult to crash servers with remoteSetCode either, but it's in an extension which is disabled by default so that reduces how strict we need to be on that a little bit at least.
edit: or alternatively if you can tie the ops cost into the compiler+parser itself so the ops increases based on how complex it was to parse, that'd also work.

* Changed from 500 to 1000 base ops cost
* Add prf based on tree size post parsing / optimziing
* Modify Tokenizer/Parser internals to optionally take a hook to call internally, used to add ops for each token/node (and be able to abort mid-parsing/tokenizing).
* Add half an op every 2 characters input in runString (running 500 char code will cost an extra 250 ops)
* Fix stack not decreasing on error
@Vurv78
Copy link
Member Author

Vurv78 commented Jul 31, 2022

also the ops cost of this needs to be significantly higher than 500, or have a cooldown of 1 second like remoteSetCode, or I imagine it would be exceptionally easy to crash servers with it. It's probably not that difficult to crash servers with remoteSetCode either, but it's in an extension which is disabled by default so that reduces how strict we need to be on that a little bit at least.

It has no issue with running the code, since it shares the same environment (and so the ops), however I didn't think about tokenizing/parsing costs. Forgot the internals are so painfully slow/bad.

I ended up adding hooks to the tokenizer and parser to add ops internally and abort if it hit quota, since they don't export any sort of iterator "next" functions, and I don't feel like rewriting them right now.

This shouldn't be placed in an extension which is enabled by default.

Why not? I did originally think of this, but that was because of the only issue I thought was because of people being dumb with it and taking user input into a runString (allowing using concmd, privilege escalation, or whatever).

Otherwise there's nothing super dangerous about it, "rce" would be really dumb with this considering you'd need http or something like that enabled, and that that point, the server would probably have remoteuploader or dangerous third party extensions enabled.

I also recommend putting the majority of this code in a separate function (in e2lib or something) and calling from the runstring function and also from here https://github.com/wiremod/wire/blob/master/lua/entities/gmod_wire_expression2/init.lua#L272-L284 to reduce duplicate code

Was also thinking of this. Maybe just an E2Lib.RunString function. However this is kind of specialized here in that it uses an already existing E2 environment/chip, so I wouldn't imagine it being used anywhere else.. Now this is practically not possible, considering the need to integrate quota checking alongside the tokenizer/parser/optimizer/compiler calls.

@Divran
Copy link
Contributor

Divran commented Aug 17, 2022

This shouldn't be placed in an extension which is enabled by default.

Why not? I did originally think of this, but that was because of the only issue I thought was because of people being dumb with it and taking user input into a runString (allowing using concmd, privilege escalation, or whatever).
Otherwise there's nothing super dangerous about it, "rce" would be really dumb with this considering you'd need http or something like that enabled, and that that point, the server would probably have remoteuploader or dangerous third party extensions enabled.

  1. It's basically the same thing as remoteSetCode, so it should be in the same file
  2. remoteSetCode especially was never really stress tested and has always been able to crash servers. your changes to ops cost may help but can you be sure it won't be abusable and able to lag servers? Extensions like that are usually disabled by default
  3. Not only possible lag sources but features that are considered "op" also tend to be disabled by default, such as the wiring extension.

@Divran
Copy link
Contributor

Divran commented Aug 17, 2022

Another thing you can change if you want, either in this pr or a different commit after this one, is to add the same ops checks to remoteSetCode and remoteUpload, so that the e2 doing the setting/uploading pays extra for it. And if it overrides itself, the extra ops should be applied after resetting.

@Vurv78
Copy link
Member Author

Vurv78 commented Aug 17, 2022

remoteSetCode especially was never really stress tested and has always been able to crash servers. your changes to ops cost may help but can you be sure it won't be abusable and able to lag servers? Extensions like that are usually disabled by default

I've added an option to the Tokenizer and Parser to provide a hook whenever a token/node is parsed, and am using that alongside code length and number of nodes optimized & compiled for ops. I think it should be pretty secure, maybe need an override for the compiler as well but I think it's already gimped enough. The issue with remotesetcode is since it doesn't check ops at all for code size, or tokenizing/parse time which can be abused very easily.

Another thing you can change if you want, either in this pr or a different commit after this one, is to add the same ops checks to remoteSetCode and remoteUpload, so that the e2 doing the setting/uploading pays extra for it. And if it overrides itself, the extra ops should be applied after resetting.

I'll try and do this in another PR, not too important as it isn't enabled by default ofc, seems like a pretty difficult change as i'd have to add an option to the gmod_wire_expression2 entity to run with ops while potentially replacing itself

@Vurv78
Copy link
Member Author

Vurv78 commented Aug 29, 2022

@Divran what's your status on this? If you still don't want it enabled by default I can close the PR, the whole point is pretty much to have access to a generic sort of JS eval, but right now it is hacked in and nerfed since the tokenizer/parser are so slow.

Maybe it can come later with a rewrite (of at least the tokenizer), or maybe as a compileString function when/if I add lambdas, so the op cost can be increased a lot in favor of re-using the return as a compiled lambda.

@Vurv78
Copy link
Member Author

Vurv78 commented Sep 2, 2022

Yeah I'll just go for that. This conflicts with my warning compiler changes too and I don't want to deal with that

@Vurv78 Vurv78 closed this Sep 2, 2022
@Divran
Copy link
Contributor

Divran commented Sep 2, 2022

oh sorry I missed that message, but yeah I would definitely prefer if it went into the same extension as the other similar functions

@Vurv78 Vurv78 deleted the runstring-v2 branch September 11, 2023 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants