Update DllImport name logic to support lack of "lib" prefix#1248
Update DllImport name logic to support lack of "lib" prefix#1248stephentoub merged 1 commit intodotnet:masterfrom
Conversation
src/pal/src/loader/module.cpp
Outdated
There was a problem hiding this comment.
I debated with myself whether this second case is actually a real scenario (e.g. where the developer wrote "crypto.so" and the runtime should prepend to try "libcrypto.so"). Let me know if you think I should remove it.
There was a problem hiding this comment.
Once you start down that path you are defining policy. As mentioned in the infamous #930, the proper dll name is the name without a suffix. This implies the proper name is also one without a prefix. We should probably enforce that early (now), rather than try to clean up the mess with logic later.
CoreCLR is meant to be cross platform. ".dll" and ".so" are not crossplatform. A library name with either a suffix or prefix should be considered an error in CoreCLR.
The implication being, no matter what string is there already a prefix (if any) and a suffix is added. So libcrypto.so would become "liblibcrypto.so.so".
The precedent for this is the C linker with take "-lname". There is no prefix or suffix.
There was a problem hiding this comment.
No matter what we do here we're defining policy; it's just a question of how much. Also, not all libraries begin with "lib", even on platforms where many do, and we still need to support those.
If the string "name" is supplied, I think it's very reasonable (required?) to say we try:
name
namesuffix
prefixnamesuffix
Are we agreed on that? I can't tell.
Beyond that, there's the question of whether we also support:
prefixname
That's the case I was asking about here. I'm tempted to say we don't, but I left it in the PR to have the discussion.
I also don't have an opinion on whether to check whether the prefix or suffix already exists and skip them if they do. I'm fine removing those checks, too, if that's the overall desired outcome. I see the arguments for not having them, and they make sense to me.
There was a problem hiding this comment.
My opinion on the matter is to take the string and add prefix and suffix regardless of whether they exist or not. Any 'name mangling' logic other than that should exist in the future solution for #930. This simplifies the runtime, eliminates false positives, and is deterministic independent of what file name may exist or not in the library working directory or search path. Additionaly, it makes mapping names in the future much simpler as multiple permutations do not need to be accounted for.
While there are (many) exceptions to the rule, prefix+library_name+suffix is canonical. The exceptions and eccentricities should be handled by the solution to #930. This should handle the majority of cases without any further work.
There was a problem hiding this comment.
I should expand on " is deterministic independent of what file name may exist or not in the library working directory or search path"
If you know the order the runtime tries library names, then you can take advantage of that to trick it into loading a different library than the one intended. The debate on who would do it or what they could do with it is better left for an issue report than here. This information is presented only to assist in making an informed decision on this specific item.
There was a problem hiding this comment.
Are we agreed on that?
I agree with that list, those are common cases that should catch a good chunk of p/invokes.
I'm not so sure about prefixname, I agree with @OtherCrashOverride that we should push people to stick with name and not put .dll/.so into the dllimport. I'd prefer removing prefixname for now to avoid people relying on this behavior.
There was a problem hiding this comment.
My thinking is that the two most likely cases people would use are prefix_name_suffix and name and that the plain name usage should be preferred.
So I think that the ideal order would be:
prefixnamesuffix
name
namesuffix
prefixname (for the sake of completeness)
I would not try to detect whether the prefix or suffix are already there. It makes the code difficult to read and in case developers would follow the recommended way (plain name), the expected shared lib would be loaded at the first attempt. If they use something different, then they will pay the cost for that.
Then the implementation can be simplifed considerably, essentially each case being just a snprintf (except of the 2nd case) followed by attempt to load the library, without the need for the loop and switch.
There was a problem hiding this comment.
Is there a specific case in CoreCLR where prefix+name+suffix is not adequate? If we start with that as a limitation, we can expand to search other names later if necessary without breaking any code. The converse, though, is not true. If we later want to remove the name substitutions, we chance breaking code.
There was a problem hiding this comment.
Any library that doesn't begin with "lib", such as "CoreFoundation" on OS X.
|
I pushed an update that simplifies the logic, reorders the search, and removes the existence checks (it now just always tries prepending and appending even if the prefix and suffix already exist). I kept all four cases (at least for now); there's something to be said for the consistency in being able to say "the runtime tries the provided name with and without each of the prefix and suffix", rather than having to enumerate the various cases, and there's an argument to be made that trying all four rather than just three is actually less policy. (I'll squash before merging.) |
|
LGTM |
|
👍 glad this has been implemented. |
There was a problem hiding this comment.
I wonder if we should start just with shortAsciiName as it was specified and if that fails we should proceed to trying different variants?
There was a problem hiding this comment.
That's what I had initially. There were several suggestions to change the order.
There was a problem hiding this comment.
Oh, I missed it. The entire conversion was hidden under “outdated diff” and I did not notice it. I’m sorry about that.
My suggestion was based on a thought that first of all we should try to load a library exactly as the caller specified without trying to guess what the caller meant to load and only if that fails we should try to play the game of guessing. Basically if we take your example with “CoreFoundation” (which does not have a prefix) and we first try to load “libcorefoundation.dylib” then we potentially can load wrong library and there will be no way for a developer to workaround that. So, personally, I would prefer the original order that you suggested: name, namesuffix, prefixnamesuffix, and probably prefixname.
Of course, if everyone else thinks that the current order makes more sense then I’m OK with it.
There was a problem hiding this comment.
My point for using the prefix / suffix first was to promote the pure name without any prefix / suffix as a recommended de-facto standard. The reason is that it would work well in cases the app is using a native lib that's available on multiple platforms, but obviously have different prefix and suffix on each.
There was a problem hiding this comment.
Thank you. Sounds good. As I said, I’m OK with the proposed order. I also don’t have any objections against promoting the name without any prefix/suffix but I’m not sure whether PAL is the right place for doing that. I would love to see PAL (as the lowest layer of the runtime stack) to be policy free. It is not going to be easy for an app to change it if we get it wrong.
In any case, Steve, please feel to merge your change.
Today the runtime supports automatically appending an appropriate suffix (e.g. ".so", ".dylib", etc.) for the library name provided to DllImport. This commit adds support for also prepending the standard library prefix "lib".
dd076b3 to
e3cce7c
Compare
Update DllImport name logic to support lack of "lib" prefix
This sounds great in theory. In practice, it sounds like an injection vulnerability just waiting to happen as soon as someone fails to follow the de-facto standard. If we don't treat the name, exactly as given, as the default choice, it's going to come back to bite us. |
In dotnet#1248 we added support for automatically prepending a prefix onto a DllImport name, in addition to the existing support for automatically appending a suffix. This leads to four combinations that are checked: name, prefix+name, name+suffix, and prefix+name+suffix. The current code as of dotnet#1248 first checks for prefix+name+suffix, under the assumption that it'll be the most common case, and thus we optimize for it, e.g. a developer writes "sqlite3" such that it'll become "sqlite3.dll" on Windows, "libsqlite3.so" on Linux, "libsqlite3.dylib" on OS X, etc. However, there is a debate about whether we should actually check first for exactly what the developer wrote, checking for just name before checking for prefix+name+suffix. Whereas the argument for prefix+name+suffix is performance, the argument for name is safety, in that by always checking first for what the developer explicitly typed, the developer has more control over ensuring that another library isn't injected into the search order ahead of it. This commit swaps the order of the checks for prefix+name+suffix and name, making the check for name first.
Today the runtime supports automatically appending an appropriate suffix (e.g. ".so", ".dylib", etc.) for the library name provided to DllImport. This commit adds support for also prepending the
standard library prefix "lib".
Thiis is meant to help with situations like that in #1244. It is in addition to whatever solution might be implemented to help with #444 and #930.