Major memory leak in CreateNewProc()

This forum is for general developer support questions.
Post Reply
softwarefailure
Posts: 108
Joined: Fri Feb 14, 2014 10:29 pm

Major memory leak in CreateNewProc()

Post by softwarefailure »

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

Code: Select all

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.
User avatar
colinw
AmigaOS Core Developer
AmigaOS Core Developer
Posts: 186
Joined: Mon Aug 15, 2011 9:20 am
Location: Brisbane, QLD. Australia.

Re: Major memory leak in CreateNewProc()

Post by colinw »

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()
softwarefailure
Posts: 108
Joined: Fri Feb 14, 2014 10:29 pm

Re: Major memory leak in CreateNewProc()

Post by softwarefailure »

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!
User avatar
colinw
AmigaOS Core Developer
AmigaOS Core Developer
Posts: 186
Joined: Mon Aug 15, 2011 9:20 am
Location: Brisbane, QLD. Australia.

Re: Major memory leak in CreateNewProc()

Post by colinw »

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]
User avatar
Raziel
Posts: 1015
Joined: Sat Jun 18, 2011 4:00 pm
Location: a dying planet

Re: Major memory leak in CreateNewProc()

Post by Raziel »

@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
People are dying.
Entire ecosystems are collapsing.
We are in the beginning of a mass extinction.
And all you can talk about is money and fairytales of eternal economic growth.
How dare you!
– Greta Thunberg
User avatar
colinw
AmigaOS Core Developer
AmigaOS Core Developer
Posts: 186
Joined: Mon Aug 15, 2011 9:20 am
Location: Brisbane, QLD. Australia.

Re: Major memory leak in CreateNewProc()

Post by colinw »

"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.
Last edited by colinw on Tue Feb 16, 2021 7:47 am, edited 1 time in total.
User avatar
Raziel
Posts: 1015
Joined: Sat Jun 18, 2011 4:00 pm
Location: a dying planet

Re: Major memory leak in CreateNewProc()

Post by Raziel »

@colinw

Understood, thank you very much
People are dying.
Entire ecosystems are collapsing.
We are in the beginning of a mass extinction.
And all you can talk about is money and fairytales of eternal economic growth.
How dare you!
– Greta Thunberg
Post Reply