www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - std.loader.ExeModule is unusable

reply Mike Parker <aldacron71 yahoo.com> writes:
The free standing functions in std.loader work as expected, but the 
ExeModule class is just plain broken. There are two major failings:

1) The class is auto. I don't understand the reasoning behind this at 
all. Consider this code:

**************************************************************
import std.loader;
import std.c.stdio;

typedef int function(uint) pfSDL_Init;
typedef int function() pfSDL_Quit;
pfSDL_Init				SDL_Init;
pfSDL_Quit				SDL_Quit;

void loadLib()
{
	auto ExeModule em = new ExeModule("sdl.dll");
	SDL_Init = cast(pfSDL_Init)em.getSymbol("SDL_Init");
	SDL_Quit = cast(pfSDL_Quit)em.getSymbol("SDL_Quit");
}

int main(char[][] args)
{
	ExeModule_Init();

	loadLib();
	printf("Initializing SDL\n");
	SDL_Init(0);
	SDL_Quit();	
	ExeModule_Uninit();
	return 0;
}
**************************************************************

When loadLib returns, em has gone out of scope and as it's an auto class 
the destructor is called. ExeModule's destructor releases the handle to 
the shared library, thereby invalidating the previously loaded function 
function addresses and causing an Access Violation.

2) The two-arg constructor is whacked:

**************************************************************
/// Constructs from an existing image handle
     this(in HXModule hModule, boolean bTakeOwnership)
     in
     {
         assert(null !== hModule);
     }
     body
     {
         if(bTakeOwnership)
         {
             m_hModule = hModule;
         }
         else
         {
	    version (Windows)
	    {
		char[] path = Path();
		m_hModule = cast(HXModule)LoadLibraryA(toStringz(path));
		if (m_hModule == null)
		    throw new ExeModuleException(GetLastError());
	    }
	    else version (linux)
	    {
		m_hModule = ExeModule_AddRef(hModule);
	    }
	    else
		static assert(0);
         }
     }
**************************************************************

First, when bTakeOwnership is true the same problem described in 1) 
arises. Second, when bTakeOwnership is false, the Windows version does 
something really whacky. Looking in the Path() method, you'll see the 
following:

uint cch = GetModuleFileNameA(cast(HModule_)m_hModule, szFileName, 			 
szFileName.length);
...
return szFileName[0 .. cch].dup;

The problem is that at this point, m_hModule is null. There is 
precondition check for this, but it will always fail if bTakeOwnership 
is false. If compiling without dbc you wind up loading a handle to the 
executable itself as the default behavior of GetModuleFileName with a 
null module is to store the exe name in szFileName. This will cause 
ExeModule.getSymbol to fail later on. And to top it off, the orginal 
module handle passed to the constructor is never used at all. So when 
bTakeOwnership is false, you are 100% gauranteed to fail either with an 
assertion failure in Path() in debug mode or an ExeModuleExeption in 
getSymbol() otherwise.

3) When an ExeModuleException is thrown via ExeModule.getSymbol() there 
is absolutely no way of knowing which symbol failed to load unless you 
wrap each call in a try...catch block, which just isn't practical. The 
only thing that is output is a system error code. At the very least 
ExeModule should concat the name of the symbol with the error code 
string. This also applies to the loading of the shared lib handles.

Currently I'm using the free standing functions, and was getting ready 
to submit improved exception messages when I stumbled into the other 
problems. I'll be happy to submit a more usable version (with Matthew's 
permission).
Jun 11 2004
next sibling parent reply "Matthew" <matthew.hat stlsoft.dot.org> writes:
It's a bit hard to comment, since the version in Phobos is not the version I
submitted to Walter. We had quite a disagreement on the implementation of the
class, and on the provision of free functions and class.

I'm not abrogating responsibility, but just explaining my difficulties.

If you sketch two or three scenarios, preferably complete (small) progs, I can
try them with _my_ implementation. If that's still out of whack - which may well
be so - then I'll amend my impl to your requirements (assuming they're sensible,
which I assume at the mo).

I'll also do a bit of documentation/rationale for the design (or my design of
it), and then open that for comment.

The issue of the auto-ness of the class is certainly not something I can run
away
from. Walter suggested it should be auto, and I complied without thinking about
it. Given the current - and I say current because I expect them to be relaxed
sometime in the not so distant future - rules regarding auto, I agree that the
class is flawed in this regard.

Can I say/do more? :-)

Cheers

Matthew


"Mike Parker" <aldacron71 yahoo.com> wrote in message
news:cada7o$2nnh$1 digitaldaemon.com...

 The free standing functions in std.loader work as expected, but the
 ExeModule class is just plain broken. There are two major failings:

 1) The class is auto. I don't understand the reasoning behind this at
 all. Consider this code:

 **************************************************************
 import std.loader;
 import std.c.stdio;

 typedef int function(uint) pfSDL_Init;
 typedef int function() pfSDL_Quit;
 pfSDL_Init SDL_Init;
 pfSDL_Quit SDL_Quit;

 void loadLib()
 {
 auto ExeModule em = new ExeModule("sdl.dll");
 SDL_Init = cast(pfSDL_Init)em.getSymbol("SDL_Init");
 SDL_Quit = cast(pfSDL_Quit)em.getSymbol("SDL_Quit");
 }

 int main(char[][] args)
 {
 ExeModule_Init();

 loadLib();
 printf("Initializing SDL\n");
 SDL_Init(0);
 SDL_Quit();
 ExeModule_Uninit();
 return 0;
 }
 **************************************************************

 When loadLib returns, em has gone out of scope and as it's an auto class
 the destructor is called. ExeModule's destructor releases the handle to
 the shared library, thereby invalidating the previously loaded function
 function addresses and causing an Access Violation.

 2) The two-arg constructor is whacked:

 **************************************************************
 /// Constructs from an existing image handle
      this(in HXModule hModule, boolean bTakeOwnership)
      in
      {
          assert(null !== hModule);
      }
      body
      {
          if(bTakeOwnership)
          {
              m_hModule = hModule;
          }
          else
          {
     version (Windows)
     {
 char[] path = Path();
 m_hModule = cast(HXModule)LoadLibraryA(toStringz(path));
 if (m_hModule == null)
     throw new ExeModuleException(GetLastError());
     }
     else version (linux)
     {
 m_hModule = ExeModule_AddRef(hModule);
     }
     else
 static assert(0);
          }
      }
 **************************************************************

 First, when bTakeOwnership is true the same problem described in 1)
 arises. Second, when bTakeOwnership is false, the Windows version does
 something really whacky. Looking in the Path() method, you'll see the
 following:

 uint cch = GetModuleFileNameA(cast(HModule_)m_hModule, szFileName,
 szFileName.length);
 ...
 return szFileName[0 .. cch].dup;

 The problem is that at this point, m_hModule is null. There is
 precondition check for this, but it will always fail if bTakeOwnership
 is false. If compiling without dbc you wind up loading a handle to the
 executable itself as the default behavior of GetModuleFileName with a
 null module is to store the exe name in szFileName. This will cause
 ExeModule.getSymbol to fail later on. And to top it off, the orginal
 module handle passed to the constructor is never used at all. So when
 bTakeOwnership is false, you are 100% gauranteed to fail either with an
 assertion failure in Path() in debug mode or an ExeModuleExeption in
 getSymbol() otherwise.

 3) When an ExeModuleException is thrown via ExeModule.getSymbol() there
 is absolutely no way of knowing which symbol failed to load unless you
 wrap each call in a try...catch block, which just isn't practical. The
 only thing that is output is a system error code. At the very least
 ExeModule should concat the name of the symbol with the error code
 string. This also applies to the loading of the shared lib handles.

 Currently I'm using the free standing functions, and was getting ready
 to submit improved exception messages when I stumbled into the other
 problems. I'll be happy to submit a more usable version (with Matthew's
 permission).
Jun 11 2004
next sibling parent Mike Swieton <mike swieton.net> writes:
On Sat, 12 Jun 2004 16:25:44 +1000, Matthew wrote:

 The issue of the auto-ness of the class is certainly not something I can run
away
 from. Walter suggested it should be auto, and I complied without thinking about
 it. Given the current - and I say current because I expect them to be relaxed
 sometime in the not so distant future - rules regarding auto, I agree that the
 class is flawed in this regard.
 


I take it you know something we don't about auto ;)

I'd like to request that auto not be changed. I think a 'relaxed' version of
auto would be very useful, but when you need something as strict as auto, it
would be nice to have it there.

Mike Swieton
__
Has this world been so kind to you that you should leave with regret? There
are better things ahead than any we leave behind.
	- C. S. Lewis
Jun 12 2004
prev sibling parent Mike Parker <aldacron71 yahoo.com> writes:
Matthew wrote:


 If you sketch two or three scenarios, preferably complete (small) progs, I can
 try them with _my_ implementation. If that's still out of whack - which may
well
 be so - then I'll amend my impl to your requirements (assuming they're
sensible,
 which I assume at the mo).


Thanks, Matthew. In addition to the first example I posted, there is this:

***********************************************************
import std.loader;
import std.c.stdio;

typedef int function(uint) pfSDL_Init;
typedef int function() pfSDL_Quit;
pfSDL_Init				SDL_Init;
pfSDL_Quit				SDL_Quit;

HXModule hlib;

void loadLib()
{
	auto ExeModule em = new ExeModule(hlib, false);
	SDL_Init = cast(pfSDL_Init)em.getSymbol("SDL_Init");
	SDL_Quit = cast(pfSDL_Quit)em.getSymbol("SDL_Quit");
}

int main(char[][] args)
{
	ExeModule_Init();
	hlib = ExeModule_Load("sdl.dll");
	loadLib();	
	SDL_Init(0);
	SDL_Quit();
	ExeModule_Release(hlib);	
	ExeModule_Uninit();
	return 0;
}
***********************************************************

In this scenario, an exception will be thrown when trying to load 
SDL_Init. The reason being that the ExeModule constructor will ignore 
the first parameter and actully will load a handle to the executable itself.

Changing the second constructor parameter to true will exhibit the same 
behavior as the first example I posted (Access Violation). ExeModule 
will store the hlib reference, closing it via the destructor when 
loadLib() exits. This will cause the function pointers to be invalid.

I suggest the following changes to ExeModule:

1) do away with the autoness
2) remove the Path() method
3) always store the reference to the module passed to the constructor, 
regardless of the value of bTakeOwnership
4) create a new class member (m_isOwner) to store the value of 
bTakeOwnership. Test this in the destructor and only call close() if 
m_isOwner == true.

And one last thing that would be useful is a change to 
ExeModuleException and the way it is used. Changing the second 
constructor to something like this:

this(char[] message, uint errcode)
{
     super(message ~ " [Sys Error: " ~ SysError.msg(errcode) ~ "]");
}

And then doing something like this in when a shared lib or a proc 
address canot be loaded:

throw new ExeModuleException("Failed to find shared lib symbol " ~ 
symbolName, GetLastError());

will greatly assist in debugging.
Jun 13 2004
prev sibling parent "The Dr ... who?" <thedr who.com> writes:
Gah! I'm an ignoramus.

The debate between myself and Walter were on ExeModule, not on MmFile. The
guilt,
such as it is, is all mine.

I'll try and take a look at it again right now.

"Mike Parker" <aldacron71 yahoo.com> wrote in message
news:cada7o$2nnh$1 digitaldaemon.com...

 The free standing functions in std.loader work as expected, but the
 ExeModule class is just plain broken. There are two major failings:

 1) The class is auto. I don't understand the reasoning behind this at
 all. Consider this code:

 **************************************************************
 import std.loader;
 import std.c.stdio;

 typedef int function(uint) pfSDL_Init;
 typedef int function() pfSDL_Quit;
 pfSDL_Init SDL_Init;
 pfSDL_Quit SDL_Quit;

 void loadLib()
 {
 auto ExeModule em = new ExeModule("sdl.dll");
 SDL_Init = cast(pfSDL_Init)em.getSymbol("SDL_Init");
 SDL_Quit = cast(pfSDL_Quit)em.getSymbol("SDL_Quit");
 }

 int main(char[][] args)
 {
 ExeModule_Init();

 loadLib();
 printf("Initializing SDL\n");
 SDL_Init(0);
 SDL_Quit();
 ExeModule_Uninit();
 return 0;
 }
 **************************************************************

 When loadLib returns, em has gone out of scope and as it's an auto class
 the destructor is called. ExeModule's destructor releases the handle to
 the shared library, thereby invalidating the previously loaded function
 function addresses and causing an Access Violation.

 2) The two-arg constructor is whacked:

 **************************************************************
 /// Constructs from an existing image handle
      this(in HXModule hModule, boolean bTakeOwnership)
      in
      {
          assert(null !== hModule);
      }
      body
      {
          if(bTakeOwnership)
          {
              m_hModule = hModule;
          }
          else
          {
     version (Windows)
     {
 char[] path = Path();
 m_hModule = cast(HXModule)LoadLibraryA(toStringz(path));
 if (m_hModule == null)
     throw new ExeModuleException(GetLastError());
     }
     else version (linux)
     {
 m_hModule = ExeModule_AddRef(hModule);
     }
     else
 static assert(0);
          }
      }
 **************************************************************

 First, when bTakeOwnership is true the same problem described in 1)
 arises. Second, when bTakeOwnership is false, the Windows version does
 something really whacky. Looking in the Path() method, you'll see the
 following:

 uint cch = GetModuleFileNameA(cast(HModule_)m_hModule, szFileName,
 szFileName.length);
 ...
 return szFileName[0 .. cch].dup;

 The problem is that at this point, m_hModule is null. There is
 precondition check for this, but it will always fail if bTakeOwnership
 is false. If compiling without dbc you wind up loading a handle to the
 executable itself as the default behavior of GetModuleFileName with a
 null module is to store the exe name in szFileName. This will cause
 ExeModule.getSymbol to fail later on. And to top it off, the orginal
 module handle passed to the constructor is never used at all. So when
 bTakeOwnership is false, you are 100% gauranteed to fail either with an
 assertion failure in Path() in debug mode or an ExeModuleExeption in
 getSymbol() otherwise.

 3) When an ExeModuleException is thrown via ExeModule.getSymbol() there
 is absolutely no way of knowing which symbol failed to load unless you
 wrap each call in a try...catch block, which just isn't practical. The
 only thing that is output is a system error code. At the very least
 ExeModule should concat the name of the symbol with the error code
 string. This also applies to the loading of the shared lib handles.

 Currently I'm using the free standing functions, and was getting ready
 to submit improved exception messages when I stumbled into the other
 problems. I'll be happy to submit a more usable version (with Matthew's
 permission).
Jun 13 2004