genpaku has quit [Remote host closed the connection]
genpaku has joined #jruby
razetime has joined #jruby
razetime has quit [Ping timeout: 268 seconds]
razetime has joined #jruby
razetime has quit [Ping timeout: 268 seconds]
razetime has joined #jruby
<headius>
Good morning!
<byteit101[m]>
morning. Got subspawn released last week and moved the jruby PR's forward as a result last weekend. I think they are finally now at or near completion
<enebo[m]>
byteit101: I am hoping 9.4 next week and hopefully before mid-next week
<enebo[m]>
we have not discussed a date other than before rubyconf but we need things finalized so if we find something significant we can fix it before the conference
<enebo[m]>
headius did some simple web perf so I think we are not too bad in that things did not explode
<enebo[m]>
Most of that is being really picky about style rules just to match up better with the rest of our source
<enebo[m]>
You have committed enough where I will be more picky about style
<enebo[m]>
I had one substantive question about how you knew you could split on \. in that URL but I did not spend much time. If that is guaranteed then it is likely fine (or you can add a comment)
<byteit101[m]>
hmm, I suppose you could make "filedll" or "fileso" or such, but that sounds very weird. I'll guard it though
<enebo[m]>
kares: We got another issue comment on the openssl 0.14 security thing. In re-reading the issue I got a little confused. I think 0.14.1pre had an issue and it was never actually released and 0.14 never had the security problem. Is that correct?
<enebo[m]>
byteit101: yeah if it is not possible for that to be entered incorrectly then you don't need to worry otherwise it will throw
<enebo[m]>
if it gets wrong input
<byteit101[m]>
this is coming from what users pass to ffi_lib
<byteit101[m]>
so bad input is expected
<enebo[m]>
So they could make a typo and we would crash...yeah be a little better on parsing that then
<byteit101[m]>
this method is called repatedly with a bunch of attempts at a path
<byteit101[m]>
I think ffi.rb catches lots of exceptions though
<enebo[m]>
You are really just looking for a literal '.' so if that is not there then just perhaps return
<kares[m]>
enebo[m]: havent seen the comment yet but yes your reasoning is correct, 0.14.1 had no changes in terms of security it's as secure as 0.14.9
<enebo[m]>
It is either error or skip but since you are trying multiple paths I guess I don't know...if it is bungled and you know it raise
<kares[m]>
kares[m]: sorry 0.14.0 ... am on mobile
<enebo[m]>
he seems to think 0.14.0 has an existing security warning and he specifically is asking if it is also vulnerable
<enebo[m]>
yeah it will just return an array of length 1 so name[1] will AIOOBE
<byteit101[m]>
But again, I am not using name[1], right?
<enebo[m]>
which will not be a helpful error to blanket catch
<enebo[m]>
HAHAH no are not but it is still odd
<enebo[m]>
"foo.foo" would be the search right?
<byteit101[m]>
result, yes
<enebo[m]>
"so.so" could actually load the wrong dll
<byteit101[m]>
This is the resolution code, so it should be returning a full path
<byteit101[m]>
see line 126
<byteit101[m]>
so I still think I'm safe?
<enebo[m]>
Let's just see if I am being too picky or not
<enebo[m]>
If someone just types in a name which ends up being "so" and in that same directory there is a load library so.so then it would load that
<byteit101[m]>
Incorrect
<enebo[m]>
which is quite unusual
<byteit101[m]>
this is for extracting to a temp dir
<byteit101[m]>
if someone types in "so" and "so" is in the classpath (ffi/library.rb) then we attempt to extract to /tmp/jrub-$RAND/so.so'
<byteit101[m]>
extractLibrary takes an InputStream, and outputs a absoloute path to a temporary file based on the original file name
<byteit101[m]>
I don't think there is a possibility for what you are suggesting. though I may be missing someting?
<enebo[m]>
ok. So it sounds like this is not possible.
<enebo[m]>
My comment I think stems from the notion from the PR code itself I see you doing name[0].name[0] which looks strange. In this case just canonicalizing and seeing if it is a valid file naively looks like a mistake even if it cannot cause the wrong thing to load
<byteit101[m]>
Fair enough, I was considering using File.basename, etc but I saw I would have to convert to RubyString and thought a simple dumb split would be easier
<enebo[m]>
So another question as someone who is only looking at a diff :)
<enebo[m]>
What is the return values of this method we are discussing. It will return a full path or just a name?
<byteit101[m]>
Absoloute path to the temporary file where the data was just extracted to
<byteit101[m]>
:126
<enebo[m]>
ok so it really never expects this to be an invalid file since you copied it there
<headius>
enebo: so I figured out why this wrapper isn't working... U
<headius>
I
<headius>
gah
<enebo[m]>
but the input of the file to copy would be from user input right? So in that case something earlier will notice it is not there
<headius>
I'm not sure wrapping is working at all
<byteit101[m]>
No, I mean .createTempFile on 120 kinda gives the jig away in that this is supposed to be a new file :-)
<enebo[m]>
yeah wrap = true is not working at all
<headius>
at the point where we determine how to define instance or script-level methods, we always go with self's metaclass, which isn't where methods should go in a wrapper script
<enebo[m]>
I am not sure why but I assume the solution to both would be to pass wrap: RubyModule
<enebo[m]>
ah
<byteit101[m]>
enebo: yes, file resolved on 78 and opened on line 83
<headius>
I have a one-liner that changes the method def logic to use current scope's module and that makes this work but we'll have to see what it breaks
<enebo[m]>
byteit101: ok. I guess I just needed to be guided through this since only the PR code looks like you will return something which cannot exist
<byteit101[m]>
If we can't resolve or open it, we abort on 87 and fail "normally"
<byteit101[m]>
happy to help
<enebo[m]>
ok that is fine. I just talk through things mostly to make sure you have a handle on it even if I don't
<byteit101[m]>
:-D
<byteit101[m]>
Alright, I gotta get "real" work done, I'll fix the other style issues this afternoon
<enebo[m]>
It does find a lot of issues inasmuch as it forces the author to think about what they wrote differently
<enebo[m]>
thanks for taking the time
<byteit101[m]>
My pleasure!
<headius>
going to run some stuff locally before I push this change
<enebo[m]>
headius: so do we need to do something special for self or is this something to intrinsically all over the place we are doomed
<headius>
might be possible to narrow this to only use scope's module for script defs and leave instance defs alone
<headius>
I think this would only have to change in the logic for top-level defs for this to work
<headius>
we already use cref for constants
<headius>
in any case I have the logic working as well as it can without fixing the wrapping so it at least accepts the module and puts it in the right place
<enebo[m]>
ok cool. That was the part I was worried about since it seemed to be a lot of signature changes
<enebo[m]>
Did you just deprecate boolean wrap versions?
<headius>
no
<headius>
I didn't plumb it through
<enebo[m]>
oh wait I read what you did
<headius>
yeah
<enebo[m]>
threadlocal
<enebo[m]>
ok
<enebo[m]>
not the ideal but the safest route
<headius>
to be honest we probably could plumb it through by adding another method to Library interface but it only really needs to be there for Ruby sources
<headius>
so existing exts can just keep the existing interface and continue to ignore wrapping like they always have
<headius>
at that point it didn't seem like it was worth the effort to add the interface right now
<headius>
maybe I should though so people can migrate to it... but then they just ignore it anywah
<enebo[m]>
yeah I think it would be nice to eventually just have load(name) and load(name, wrapper = Module.new)
<enebo[m]>
when? I don't know but it seems like we have a lot to do right now
<enebo[m]>
out of band definitely seems like a good solution for 9.4 to me
<enebo[m]>
At some point we need to just put "deprecate" on our entire code base
<enebo[m]>
We have a lot of debt associated with not knowing what people are consuming and we sort of need a do over
<enebo[m]>
I am not suggesting a grand rewrite as much as just again noting we have a "mature" code base
<headius>
yeah
<headius>
ok I pushed the change to DefInstMethod
<headius>
or rather the change to the logic to find where methods get defined for top-level