[Openmp-commits] [PATCH] D73657: [OPENMP] Load plugins from same directory as the libomptarget.so and quick fail mechanism for offloading plugins

[Joachim Protze via Phabricator via Openmp-commits]

protze.joachim added a comment.

Not sure whether greg still plans to work on this. Anyway a inline comment to the PATH_MAX



================
Comment at: openmp/libomptarget/src/rtl.cpp:72
+    DP("dlopen() failed: %s\n", dlerror());
+  char *libomptarget_dir_name = new char[256];
+  if (dlinfo(handle, RTLD_DI_ORIGIN, libomptarget_dir_name) == -1)
----------------
lebedev.ri wrote:
> jdoerfert wrote:
> > gregrodgers wrote:
> > > JonChesterfield wrote:
> > > > ABataev wrote:
> > > > > Why need to use dynamic allocation here?
> > > > To keep it off the stack, but char[256] seems fine as a local to me too.
> > > > 
> > > > Is there a cross platform equivalent to this in the llvm libraries? Seems a bit suspect that dlinfo doesn't take the size of the buffer it writes to.
> > > > 
> > > > Plus I thought PATH_MAX was 256 and Google suggests Linux sets it to 260 now, so a wrapper with associated ready made error handling would be nice.
> > > Sadly, nothing in the OpenMP runtime uses llvm libraries.    
> > > We don't know the size till dlinfo gives us the name.  Hmmm, maybe there is a dlinfo on the size of the name.
> > This all sounds scary. I am by far no expert in this but maybe `PATH_MAX` is the right size here, see als `llvm/lib/Support/Unix/Path.inc`
> It is most certainly not `260`, and not 256 on linux, but 4095.
> On hurd it is righfully not defined at all - the code should not be making assumptions
> about the upper bound on it's length, because clearly it is easy to fall short.
> 
260 seems to be the limit on windows according to this blog article: http://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html

Since dlinfo does not use a size argument, I'd consider this prone to buffer overflow. Or in the best case you get a truncated path.
`RTLD_DI_LINKMAP` might be the better choice, as it should provide you access to the original name.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73657/new/

https://reviews.llvm.org/D73657