<lavamind[m]>
Oh this was already in the log I sent, I thought I had needed to trim it
<headius>
ok
olleolleolle[m] has joined #jruby
* olleolleolle[m]
waves all around, smiling and bobbing his head
<olleolleolle[m]>
https://github.com/rubygems/gemstash/actions/runs/3889717845/jobs/6638217927 I am looking at gemstash's use of Psych, with JRuby 9.4. A NoMethodError "undefined method `parse' for #<Psych::Parser:0x388623ad> /home/runner/.rubies/jruby-9.4.0.0/lib/ruby/stdlib/psych.rb:455:in `parse_stream'" - hmmm.
<headius>
so my perf experiments yesterday seemed to help even though they were trivial
<headius>
I've pushed a branch to my repo called "optz"... the only changes in there currently are caching the lookup from a few super forms and refined calls but it dropped method lookup in profiling to about half what it was
<headius>
time per iteration of railsbench went from mid 1400ms to mid/low 1300ms... without any inlining or indy wiring
<headius>
I'm going to pivot back to issues today but this seems promising... the top profile issues all can be greatly improved
<enebo[m]>
yeah that sounds good
<enebo[m]>
I am also taking a swipe at removing the bifurcation due to **empty_kwargs
<enebo[m]>
for generated code it is not probably much of a difference for speed but it makes all kwargs callsites larger
<headius>
yeah I saw a ton of hash overhead in the profile too so that is an area we need to improve
<headius>
including uncached lookups coercing objects to hash
<enebo[m]>
well hash itself is complicated without punching through ordered args instead of hashes
<headius>
yeah for sure
<enebo[m]>
but with that said a lot of it will always be hashes
<headius>
need to make sure it is using the single-bucket hash for those
<enebo[m]>
yeah that would be a good improvement
<enebo[m]>
and something we can do sooner than later
<enebo[m]>
I just fixed another kwarg report...yield(a: 1) was not being made a kwarg because we have that uwrap behavior
<headius>
ah yeah
<enebo[m]>
I had a fixme by it saying "I bet this won't work with only keywors" HAHAHA
<enebo[m]>
but I cannot be expected to notice a stray comment from 2014
<headius>
foo(a:1,b:2,c:3) does appear to be a single-bucket hash
<enebo[m]>
smallHash?
<headius>
we need to split our Hash impl into a base abstract and some different impls... we could make this tiny when we know it's a small kwargs hash
<headius>
just an array of IRubyObject key, value, key, value for example
<headius>
or a specialized Hash3 that has three key and three value fields
<headius>
yeah smallHash logic
<headius>
it is still like five objects though
<enebo[m]>
hmm in interp it is RubyHash.newHash
<headius>
RubyHash, array[1], and three hash entry objects
<headius>
hmm
<enebo[m]>
kwargsHash is a thing in arguments compiler
<headius>
I will try interp
<enebo[m]>
I bet you do small hash
<headius>
woah yeah
<headius>
interp is not doing it
<headius>
bucket array is 11 wide
<enebo[m]>
heh
<enebo[m]>
Though this should be a bit smarter since Hash is also a Hash literal
<headius>
small hash literals should also use this
<enebo[m]>
and one which can grow. So probably just same logic as compiler where it is only for kwargs
<enebo[m]>
I am not so sure
<enebo[m]>
unless that hash swaps to more buckets it could be bad to pass that literal to a method which adds 2000 more keys
<headius>
it would immediately rehash if modified in most cases
<headius>
and spread out the buckets
<enebo[m]>
ok then why not always use smallHash
<enebo[m]>
I suppose this change would do that
<headius>
well if it starts with 100 entries it's a linear search to look anything up
<enebo[m]>
we do have pairs.length so this is not worth discussing :)
<enebo[m]>
but it may make sense to have some smarter method in RubyHash which picks for us
<enebo[m]>
that new method could also use any eventual shaping
<enebo[m]>
The feature is you can load anything in your classloader PERIOD. But I may accept random untrusted data which does something bad. Ok?
<enebo[m]>
They literally do not understand there is no solution to that
<enebo[m]>
unless they want a new feature with a whitelist even then it could still be a DOS
<headius>
I think I can dodge this issue by using this SafeConstructor mentioned along the way
<enebo[m]>
but even with CL you could restrict types by just putting in a CL which can only see some classes
<headius>
basically fix Psych to configure SnakeYAML to not do any Java object support
<headius>
which is what he mentions as current mitigation
<enebo[m]>
Yeah we are not loading Java so seems safe
<enebo[m]>
oh heh... I said that before I made it to whitelist discussion
<enebo[m]>
this is epic
<enebo[m]>
Let's face it YAML supporting object extensions was always a huge mistake
<headius>
I'm not sure we even use this logic
<enebo[m]>
We must supoprt !ruby or whatever that syntax is
<enebo[m]>
but I imagine that is us making the instances directly
<enebo[m]>
which would likely be the same CVE in a difference form as this snakeYAML issue since untrusted data could conceivably create any Ruby instance
<headius>
that is done at the Psych level though
<enebo[m]>
yeah I am not saying that is our snakeyaml problem but just that it is a generic problem of YAML allowing this in the first place
<enebo[m]>
Psych will end up with this report
<headius>
and I think they've mostly mitigated that in recent versions (unsupported by default)
<headius>
yeah for sure
<enebo[m]>
yeah turning it off is the only way to make it safe
<headius>
it is a problem for any markup that can be used to arbitrarily create objects
<enebo[m]>
yeah it is the basic issue. but it is the feature. So I still cannot see how snakeYAML can do anything about it
<enebo[m]>
but I am not done reading either :)
<enebo[m]>
"This call chain is possible, not because of a misbehaving constructor, but because SnakeYaml calls hashCode on an untrusted object, leading to remote code execution."
<enebo[m]>
HAHAHAH wow
<headius>
an untrusted object whos code you have already loaded into the VM
<enebo[m]>
"I like the solution where whatever the problem code is, is just disabled by default. Just my two cents."
<enebo[m]>
I like this guy
<headius>
he has multiple paragraphs and links on the main project page about these CVEs
<headius>
yeah I don't think we are affected
<headius>
we don't even use the yaml serialization system from SnakeYAML, we use the parser directly and just pass its nodes to Psych Ruby code
<enebo[m]>
' 00% of the application which use SnakeYAML do not parse data from untrusted sources.
<enebo[m]>
I’m honestly baffled by this claim. This CVE and the so called “low quality tooling” pointed us to a huge security issue in our application. We use SnakeYaml to parse somewhat complex configuration files and while those can only be accessed by admins of the application, that does not mean that they are trusted to execute arbitrary code on a production system.'
<enebo[m]>
I don't understand this. They have admins who write config files and are worried they will attack the production system?
<enebo[m]>
Perhaps those "admins" are just customers? If people are worried employees can inject and attack their own systems then I think they maybe have another issue
<enebo[m]>
so to reiterate. I do think YAML should not support this and for this guy's sanity he should just toggle it off by default with a simple way for people to enable it (which most people do not need it)
<enebo[m]>
The YAML police will not come after him but these people will never stop reporting this and it is clear the guy is seriously burnt out
<headius>
yeah this is all based on using his high-level entry point, the Yaml class, to dump and load YAML from/to Java objects
<headius>
we don't do that
<headius>
oh hey I had an epiphany about the yarp parser... by the time it's usable we may have Panama for real in an LTS JDK... we can use that to generate a wrapper on the fly without shipping additional JNI code
<enebo[m]>
yeah I think so too re YARP but I am currently suggesting the JNI bindings are part of YARP
<enebo[m]>
We can connect to the .so however we want though
<headius>
21 out this fall will be the next LTS
<headius>
yeah
<enebo[m]>
I almost did it with FFI for my POC
<enebo[m]>
but benoit already had made the jni so I just used that
<enebo[m]>
It literally binds one void method with no args and one method byte[], byte[]
<enebo[m]>
Not too important but I am happy to see this get a little smaller in size
<headius>
enebo: ah yeah nice
<headius>
I realized I could also embed these into the indy call path and eliminate the setCallInfo call from bytecode
<enebo[m]>
The basic handling logic is still in 3 places so I may get that smaller but there are subtle differences
<headius>
if it were an attribute of the call instr that would be easier to do
<enebo[m]>
yeah in MRI these are embedded into the call site data itself
<headius>
right
<enebo[m]>
this is much more of a make it fit
<headius>
makes sense and we'll need to do that for kwarg plumbing later anyway
<enebo[m]>
and my current dirty tree is eliminate the keywords = brittleness in native methods
<headius>
all calls will need a kwarg descriptor at some point, but that can just be the call info right now
<enebo[m]>
I am going to not erase callinfo in native methods but mark callinfo it was in a native method
<enebo[m]>
if it goes to a normal ruby method that method will see what it passed still so all passthroughs will just work
<headius>
ah yeah
<enebo[m]>
native -> native calls will know it should erase callinfo unless the second native call is keywords
<enebo[m]>
the only problem here is I reversed the brittleness
<enebo[m]>
any calls which do not take keywords will still think they are passed
<enebo[m]>
(in native methods which happen to callMethod)
<enebo[m]>
In my mind, at least, I believe no method we callMethod will actually be processing kwargs unless they expect them
<enebo[m]>
If they expect their own we have to set that up in Java and we will mark callinfo for that to work
<enebo[m]>
but I do not know of any direct java to ruby calls we make via callMethod which would expect or get data from the original method and expact it to be not a kwarg
<enebo[m]>
That perhaps sounds a little confusing but the conclusion is I think the number of places where we see brittleness in reversing this will be ~0
<headius>
once we have a kwarg call path, this will just go away
<enebo[m]>
well yeah callInfo should go away. It is too clever
<headius>
it will be implied by calling along the kwarg call path anyway
<enebo[m]>
I am not giving myself credit there but I am just saying making something which is per callsite and making it a single value per thread is pretty complicated
<headius>
yeah
<headius>
well, moving the current callInfo into the call site wouldn't be too hard
<headius>
just make it an attribute of all call instructions and I can embed it into the call site (or just emit what we have now for non-indy call path)
<enebo[m]>
the main problem is where we have callInfo from a call (think interp) and where we have access to either indy of a CallSite object itself
<enebo[m]>
for JIT this is not hard because you have it all
<enebo[m]>
In the actual call itself it needs to be able to ask for it.
<enebo[m]>
MRI solves some of the complexity by having new call_keywords sorts of methods
<headius>
right
<enebo[m]>
so ec I think gets more easily accessible (it has been a long time since I looked)
<enebo[m]>
but I am happy to see this get replaced and you may remember I also want native calls to be able to receive their callsite
<enebo[m]>
or to be able to access it
<enebo[m]>
with generic cache storage
<enebo[m]>
then sprintf and strftime and regexp (etc) can just save things off without needing special stuff
<enebo[m]>
Even if this was @JRubyMethod(needsSiteCache=true) I think it would be cool
<headius>
subspawn fixes this so an argument could be made for backporting it into 9.3, but I think my comment is right about just using subspawn on 9.3 to replace spawn
<headius>
I guess I could fix our legacy spawn though too
<byteit101[m]>
But subspawn shouldn't be used unless users are requiring it themselves
<byteit101[m]>
also, require-builtin isn't the public API, it is for ruby impls to call. I commented on the ticket
<headius>
if someone wanted to use subspawn to replace spawn, though, that's the simplest way, no?
<byteit101[m]>
subspawn/replace not subspawn/replace-builtin
<headius>
ok
<byteit101[m]>
replace-builtin doesn't pull in PTY stuff