Hyperion Entertainment Message Boards

Posted: **Sat Feb 06, 2021 2:07 pm**

Check out the attached demo code. I think there's a major memory leak in CreateNewProc() when using these lines:

NP_Output, Output(),
NP_Input, Input(),
NP_CloseOutput, 0,
NP_CloseInput, 0,

without those lines, there is no leak. Also when just one thread is involved, there is no leak either, i.e. if you disable this line, it doesn't leak either:

Code: Select all

// disable the next line and it won't leak
flag = !flag;

Here's the demo program. It leaks about 750kb of memory here per start.

Code: Select all

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#include <string.h>

#include <dos/dos.h>
#include <dos/dostags.h>
#include <exec/exec.h>

#include <proto/dos.h>
#include <proto/exec.h>

#define STC_STARTUP  -2
#define STC_SHUTDOWN -1

struct Library *DOSBase = NULL;
struct DOSIFace *IDOS = NULL;

struct SubTaskMsg
{
	struct Message stm_Message;
	WORD stm_Command;
	APTR stm_Parameter;
	LONG stm_Result;
};

struct SubTask
{
	struct Task *st_Task;
	struct MsgPort *st_Port;
	struct MsgPort *st_Reply;
	APTR st_Data;
	struct SubTaskMsg st_Message;
	BPTR fh;
};

static LONG SendSubTaskMsg(struct SubTask *st, WORD command, APTR params)
{
	st->st_Message.stm_Message.mn_ReplyPort = st->st_Reply;
	st->st_Message.stm_Message.mn_Length = sizeof(struct SubTaskMsg);
	st->st_Message.stm_Command = command;
	st->st_Message.stm_Parameter = params;
	st->st_Message.stm_Result = 0;

	PutMsg((command == STC_STARTUP) ? &((struct Process *) st->st_Task)->pr_MsgPort : st->st_Port, (struct Message *) &st->st_Message);
	WaitPort(st->st_Reply);
	GetMsg(st->st_Reply);

	return st->st_Message.stm_Result;
}

static APTR createproc(STRPTR name, APTR func, APTR data, APTR replyport, int stacksize, int prio)
{
	struct SubTask *st;

	st = AllocVec(sizeof(struct SubTask), MEMF_PUBLIC|MEMF_CLEAR);
	if(st) {

		st->st_Reply = replyport;
		st->st_Data = data;
		st->st_Task = (struct Task *) CreateNewProcTags(NP_Entry, (ULONG) func,
			NP_Name, (ULONG) name,
			NP_StackSize, (ULONG) stacksize,
			NP_Output, Output(),
			NP_Input, Input(),
			NP_CloseOutput, 0,
			NP_CloseInput, 0,
			TAG_DONE);

		if(st->st_Task) {
			if(SendSubTaskMsg(st, STC_STARTUP, st)) return st;
		}

		FreeVec(st);
	}

	return NULL;
}

static void killproc(APTR handle)
{
	struct SubTask *st = (struct SubTask *) handle;

	SendSubTaskMsg(st, STC_SHUTDOWN, st);
	FreeVec(st);
}

static void exitproc(APTR handle, int failed)
{
	struct SubTask *st = (struct SubTask *) handle;

	if(st->st_Port) DeleteMsgPort(st->st_Port);
	if(st->fh) Close(st->fh);

	// We reply after a Forbid() to make sure we're really gone when the main task continues.
	Forbid();
	st->st_Message.stm_Result = FALSE;
	ReplyMsg((struct Message *) &st->st_Message);
}

static void setprocready(APTR handle)
{
	struct SubTask *st = (struct SubTask *) handle;

	st->st_Message.stm_Result = TRUE;
	ReplyMsg((struct Message *) &st->st_Message);
}

static APTR initproc(APTR userdata, int debug)
{
	struct Task *me = FindTask(NULL);
	struct SubTask *st;
	struct SubTaskMsg *stm;

	WaitPort(&((struct Process *) me)->pr_MsgPort);
	stm = (struct SubTaskMsg *) GetMsg(&((struct Process *)me)->pr_MsgPort);
	st = (struct SubTask *) stm->stm_Parameter;

	if(!(st->st_Port = CreateMsgPort())) {
		exitproc(st, FALSE);
		return NULL;
	}

	if(debug) st->fh = Open("CON:640/0/400/480/Proc Debug", MODE_NEWFILE);

	return st;
}

static void prepprocexit(APTR handle)
{
	struct SubTask *st = (struct SubTask *) handle;

	GetMsg(st->st_Port);
}

static SAVEDS void sub_proc(void)
{
	struct SubTask *st = initproc(NULL, FALSE);
	ULONG quitsig;

	setprocready(st);

	quitsig = 1L << ((struct SubTask *) st)->st_Port->mp_SigBit;

	for(;;) {
		ULONG sigmask = Wait(quitsig);
		if(sigmask & quitsig) break;
	}

	prepprocexit(st);
	exitproc(st, FALSE);
}

int main(int argc, char *argv[])
{
	struct MsgPort *replyport;
	APTR handle[2] = {NULL, NULL};
	int k, flag = 0;

	DOSBase = OpenLibrary("dos.library", 0);
	IDOS = (struct DOSIFace *) GetInterface(DOSBase, "main", 1, NULL);

	replyport = CreateMsgPort();

	for(k = 0; k < 3000; k++) {

		if(!(handle[flag] = createproc("My subtask", (APTR) sub_proc, NULL, replyport, 32768, 0))) {
			printf("Error!\n");
			return 0;
		}

		// disable this line and it won't leak
		flag = !flag;
		
		if(handle[flag]) killproc(handle[flag]);
	}

	flag = !flag;
	if(handle[flag]) killproc(handle[flag]);
			
	DeleteMsgPort(replyport);

	DropInterface((struct Interface *) IDOS);
	CloseLibrary(DOSBase);

	return 0;
}

On MorphOS and AmigaOS3 there is no leak. Only OS4 is affected. I've tested various versions of OS4.1 including the most recent version (Final Edition Update 2) and the leak happens on all of them.

Posted: **Mon Feb 08, 2021 12:46 am**

What a wonderfull little Rube Goldberg machine you have there.
The reason the memory is leaking is available from the serial debug output. (level 3+) ;

Code: Select all

 "[DOS] INFO; run() input fh buffer was changed by; My subtask [0x57E2D4F0 Vs 0x576C27F0]

And then the internal runcommand function dumps the Input() filehandle buffer because it thought
someone must have called SetFilehandleAttr() with FH_BufferSize change tag, when the new process started up.
( SetFilehandleAttr() frees the old buffer if you change it with that function. )
Instead, one of your subprocesses indirectly changed it, and didn't free the old buffer when it returned
to restore the original Input() filehandle buffer, which now has a different buffer address from before
because it was modified back by the previous process, when it returned to DOS.

The buffer change is to make ReadArgs() and friends work with the Input() stream for each new process.
The Input() stream is not posix compliant, never was in AmigaDOS, so you can't use the same buffered
input stream across multiple processes. One per customer only.

You shouldn't be doing it with the Output() stream either, and the only way you are not crashing your
brains out, is because I added a (posix like) semaphore to the buffered I/O stream handles in V51.45
to try and mitigate some of the abuse. See; SetFilehandleAttr(FH_Locking,...);

So...
Do not specify a NP_Input tag at all, and you will get a nice and shiny new NIL: stream by default.
Do not share NP_Output, make a duplicate of it with DupFileHandle() for each new process, if you must,
or simply don't specifiy it at all, for an automatic NIL: stream.
Do not specify NP_CloseOutput, 0, NP_CloseInput, 0,
If you follow the above instructions then you won't leak memory.

Also, don't forget that you should specify NP_Child,TRUE as all the child processes are sharing the
parents code segment and will crash the machine if the parent unloads first.
See also; IDOS->WaitForChildExit()

Posted: **Mon Feb 08, 2021 3:29 pm**

Thanks, I'm now using

Code: Select all

NP_Output, DupFileHandle(Output()),
NP_Input, DupFileHandle(Input()),

and no NP_CloseOutput/NP_CloseInput and the leak is gone. Thanks a lot!

Posted: **Thu Feb 11, 2021 11:58 pm**

PS: I forgot to mention before that it is important to make sure that
the proto/ files are always included FIRST.

#include <proto/dos.h>
#include <proto/exec.h>
[ ... everything else afterwards]

Posted: **Fri Feb 12, 2021 8:58 am**

@colinw

Could you elaborate why?

Is that a special case in just this situation/program or a general advice?

Do they have to be included first *always always*, no matter if it's a port or a native program?

Thank you

Posted: **Tue Feb 16, 2021 3:24 am**

"Always always" !

If you have a look at the proto files, you will see why.

Not only do they take care of implementing the various switches available to use
for the type of program method you require, by pre-declaring various definitions like;
__NOLIBBASE__
__NOGLOBALIFACE__
__USE_INLINE__
__USE_BASETYPE__

But it also automatically sets up for C++ and different C compilers quirks too.

Besides that, some of the include files have dependancies that will cause issues
if you include them in the wrong order.

Most are ok, but a recent one that was found was; (v54.101) dos/startup.h ( on line 45)
where ExecBase had to be forward referenced because it would be problematic if
exec/execbase.h was not included first.

So, the moral to this story is, don't include anything before the proto files, and only include
extras afterwards if they are actually needed.

Posted: **Tue Feb 16, 2021 7:02 am**

@colinw

Understood, thank you very much

Hyperion Entertainment Message Boards

Major memory leak in CreateNewProc()

Major memory leak in CreateNewProc()

Re: Major memory leak in CreateNewProc()

Re: Major memory leak in CreateNewProc()

Re: Major memory leak in CreateNewProc()

Re: Major memory leak in CreateNewProc()

Re: Major memory leak in CreateNewProc()

Re: Major memory leak in CreateNewProc()