Serious low memory handler bug (Solved)

This forum is for general developer support questions.
xenic
Posts: 1185
Joined: Sun Jun 19, 2011 12:06 am

Serious low memory handler bug (Solved)

Post by xenic »

While working on the low memory handler in Dopus5, I discovered a fatal bug in OS4. If a low memory handler returns MEM_TRY_AGAIN then the system will fail and freeze. I've written this test program to demonstrate the freeze: (NOTE: Example test program changed from that in original post)

Code: Select all

// compile with: gcc handlertest.c -o handlertest -lauto

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

#pragma pack(2)
struct MemData
{
	char               null_string[4];
	struct Interrupt   low_mem_handler;
	short              low_mem_signal;
	short              release_delay;
	long               call_count;
};
#pragma pack()

struct MemData *data = NULL;

int32 low_mem_handler(struct ExecBase *ExecBase,
                    struct MemHandlerData *memh,
                    struct MemData *data);


int main(int argc, char **argv)
{
	uint32 signals = 0;

	signal(SIGINT, SIG_IGN); // Tell C to ignore some signalss
	if (!(data = IExec->AllocVecTags(sizeof(struct MemData),
	                                AVT_Type, MEMF_SHARED,
	                                AVT_ClearWithValue, 0,
	                                TAG_END)))
	{
		return RETURN_ERROR;
	}

	data->low_mem_signal = 1;
	data->call_count = 0;

	// Initialise interrupt
	data->low_mem_handler.is_Node.ln_Pri=50;
	data->low_mem_handler.is_Node.ln_Type=NT_EXTINTERRUPT;
	data->low_mem_handler.is_Node.ln_Name="dopus memhandler";
	data->low_mem_handler.is_Data=data;
	data->low_mem_handler.is_Code=(void (*)())low_mem_handler;

	// Add the handler
	IDOS->Printf("Adding Handler\n");
	IExec->AddMemHandler(&data->low_mem_handler);

	signals = IExec->Wait(SIGBREAKF_CTRL_C);

	// Remove low-memory handler
	if (data->low_mem_handler.is_Node.ln_Pri==50)
		IExec->RemMemHandler(&data->low_mem_handler);

	IDOS->Printf("Handler call count: %ld\n", data->call_count);
}

// Low memory handler
int32 low_mem_handler(struct ExecBase *ExecBase,
                    struct MemHandlerData *memh,
                    struct MemData *data)
{
	// Is this the first time?
	if (!(memh->memh_Flags&MEMHF_RECYCLE))
	{
		// Just doing something for test purposes
		if (data->low_mem_signal>-1)
		{
			// Increment call count
			data->call_count++;

			// Tell it to try again
			return MEM_TRY_AGAIN;
		}
	}

	// We didn't actually do anything
	return MEM_DID_NOTHING;
}
Download and install the "AllocMem" progam from OS4Depot.
Compile the above program and execute it in a shell.
For an X1000 with 2GB memory enter allocmem 1300 MB. For other hardware you may need to try smaller numbers untill allocmem returns with "Error: Memory allocation failed!".
The system (tested on an X1000 only) will freeze and a hard reset is necessary.

The MEM_TRY_AGAIN return value for a low memory handler may not be as useful since exec is no longer setting the MEMHF_RECYCLE flag in MemHandlerData->memh_flags but OS3 programs, older OS4 programs and new programs that want to release memory incrementally could still use the MEM_TRY_AGAIN return value. It may seem unlikely that a low memory condition will occur on PPC hardware with a large amount of memory but some programs like OS3 Adpro intentionally attempt to allocate more memory than is available in order to trigger low memory handlers into freeing up as much memory as possible.

The takeaway from this is the following:
1. Programs (especially OS3 programs) that use low memory handlers which rely on the MEMHF_RECYCLE flag being properly set may fail or react unpredictably.

2. Programs that use low memory handlers which return MEM_TRY_AGAIN under low memory conditions will cause a complete system failure requiring a hard reset.

3. The exec AddMemHandler autodoc needs to be updated with a note that exec not longer sets the MEMHF_RECYCLE flag and that program low memory handlers should no longer rely on that flag being set. The tiny note to that effect in the exec includes is easily overlooked and may not get programmers attention.
Last edited by xenic on Sun Aug 30, 2015 3:24 pm, edited 2 times in total.
AmigaOne X1000 with 2GB memory - OS4.1 FE
User avatar
samo79
Posts: 572
Joined: Fri Jun 17, 2011 11:13 pm
Location: Italy

Re: Serious low memory handler bug (freeze)

Post by samo79 »

Maybe your problem is different, maybe for any other reason ... but it seems similar to our old reports related to the system in low memory condition

http://forum.hyperion-entertainment.biz ... mory#p4318

That report referred to the old OS 4.1 update 3 but nothing seems changed even on the Final Edition, on my computer atleast...
User avatar
tboeckel
AmigaOS Core Developer
AmigaOS Core Developer
Posts: 68
Joined: Mon Jun 20, 2011 8:56 am
Contact:

Re: Serious low memory handler bug (freeze)

Post by tboeckel »

xenic wrote:While working on the low memory handler in Dopus5, I discovered a fatal bug in OS4. If a low memory handler returns MEM_TRY_AGAIN then the system will fail and freeze. I've written this test program to demonstrate the freeze:
There is no bug in OS4, but in your code and you get exactly what you are asking for: your handler is called again and again. The condition "data->low_mem_signal>-1" never evaluates to "FALSE" and hence your handler always returns MEM_TRY_AGAIN which in turn lets Exec call it again as requested. If you want to avoid this infinite calling then you should return something else at some point or at least should make sure that "data->low_mem_signal" eventually becomes negative.
xenic
Posts: 1185
Joined: Sun Jun 19, 2011 12:06 am

Re: Serious low memory handler bug (freeze)

Post by xenic »

tboeckel wrote: There is no bug in OS4, but in your code and you get exactly what you are asking for: your handler is called again and again. The condition "data->low_mem_signal>-1" never evaluates to "FALSE" and hence your handler always returns MEM_TRY_AGAIN which in turn lets Exec call it again as requested. If you want to avoid this infinite calling then you should return something else at some point or at least should make sure that "data->low_mem_signal" eventually becomes negative.
You are right in a sense, because I posted the wrong version of my test program. Please look at the changed test program. In OS3 the MEMHF_RECYCLE flag is tested so that the handler will not continue to return MEM_TRY_AGAIN. As noted in SDK:Include/include_h/exec/memory.h, OS4 no longer sets the MEMHF_RECYCLE flag. As a result, OS3 programs that have low memory handlers (like Dopus5) and rely on the MEMHF_RECYCLE flag to know if the handler has already been called, will (as you already pointed out) continue to be called again.

The note about the MEMHF_RECYCLE in the exec includes can easily be overlooked and new programs could use the old handler technique in new code. Without the MEMHF_RECYCLE flag to indicate to a handler that it has already been called once, the MEM_TRY_AGAIN return value is useless anyway.

The bug is that without the MEMHF_RECYCLE flag being set by exec, older programs that use the MEM_TRY_AGAIN return value will freeze the system. The current MEM_TRY_AGAIN action by exec should have been disabled when the setting of the MEMHF_RECYCLE flag was removed.

I hope I have explained the problem clearly enough this time.
AmigaOne X1000 with 2GB memory - OS4.1 FE
User avatar
gazelle
Posts: 102
Joined: Sun Mar 04, 2012 12:49 pm
Location: Frohnleiten, Austria

Re: Serious low memory handler bug (freeze)

Post by gazelle »

It's my understanding that at some point the memhandler should return MEM_ALL_DONE (or MEM_DID_NOTHING) if it is called repeatedly and not rely on MEMHF_RECYCLE.

BTW: The MEMHF_RECYCLE is #ifdef enclosed, so you should get a compiler error if you use it.
xenic
Posts: 1185
Joined: Sun Jun 19, 2011 12:06 am

Re: Serious low memory handler bug (freeze)

Post by xenic »

gazelle wrote:It's my understanding that at some point the memhandler should return MEM_ALL_DONE (or MEM_DID_NOTHING) if it is called repeatedly and not rely on MEMHF_RECYCLE.

BTW: The MEMHF_RECYCLE is #ifdef enclosed, so you should get a compiler error if you use it.
If you look at the test code you'll see that it would return MEM_DID_NOTHING the second time it's called after returning MEM_TRY_AGAIN if the MEMHF_RECYCLE flag was being set for the second call.

Here's the way it worked on OS3 hardware:
The handler is called with the MEMHF_RECYCLE flag cleared.
The handler checks for the MEMHF_RECYCLE flag, releases memory if it's not set and returns MEM_TRY_AGAIN.
Exec trys the allocation again and the handler get called with the MEMHF_RECYCLE flag set.
The handler check the MEMHF_RECYCLE flag and because it's set, the handler returns MEM_DID_NOTHING.

My point is that OS3 programs that use that method in low memory handler are going to freeze the system because OS4 exec no longer sets the MEMHF_RECYCLE flag. I just fixed the low memory handler code in Dopus5 because nobody on the open source team was aware that using the -D__OBSOLETE_MEMHANDLER_FLAGS__ definition in the makefile and using MEM_TRY_AGAIN like the original OS3 code would cause a failure because the system no longer sets the MEMHF_RECYCLE flag. That fact should be in the AddMemHandler() autodoc.

Who knows how many OS3 programs have similar low memory handler code and will freeze the system under low memory conditions. If the OS4 devs insist that it's not a problem and prefer not to prevent potential system freezes then so be it. How hard could it be to change exec to treat a MEM_TRY_AGAIN return value the same as a MEM_ALL_DONE return value or MEM_DID_NOTHING return value and solve a potential freeze. I've done all I can to make people aware of the problem.
AmigaOne X1000 with 2GB memory - OS4.1 FE
User avatar
salass00
AmigaOS Core Developer
AmigaOS Core Developer
Posts: 530
Joined: Sat Jun 18, 2011 3:12 pm
Location: Finland
Contact:

Re: Serious low memory handler bug (freeze)

Post by salass00 »

xenic wrote: If you look at the test code you'll see that it would return MEM_DID_NOTHING the second time it's called after returning MEM_TRY_AGAIN if the MEMHF_RECYCLE flag was being set for the second call.
What gazelle meant is that there is no reason for a MemHandler to rely on MEMHF_RECYCLE to do this.

If there is memory that the handler can free it should do so and return MEM_TRY_AGAIN or MEM_ALL_DONE depending on if there is still more memory that can be freed or not. If there was no memory available to free it should return MEM_DID_NOTHING.
User avatar
gazelle
Posts: 102
Joined: Sun Mar 04, 2012 12:49 pm
Location: Frohnleiten, Austria

Re: Serious low memory handler bug (freeze)

Post by gazelle »

xenic wrote:The handler is called with the MEMHF_RECYCLE flag cleared.
The handler checks for the MEMHF_RECYCLE flag, releases memory if it's not set and returns MEM_TRY_AGAIN.
Exec trys the allocation again and the handler get called with the MEMHF_RECYCLE flag set.
The handler check the MEMHF_RECYCLE flag and because it's set, the handler returns MEM_DID_NOTHING.
Why does the function not return MEM_ALL_DONE in the frist place if it don't want do be called a second time? That's just a bug in the program. It should only return MEM_TRY_AGAIN if it actually has something to free if it is called a second time.
xenic wrote:My point is that OS3 programs that use that method in low memory handler are going to freeze the system because OS4 exec no longer sets the MEMHF_RECYCLE flag.
I understand your point but still think the program is at fault, not the OS.
xenic wrote:I just fixed the low memory handler code in Dopus5 because nobody on the open source team was aware that using the -D__OBSOLETE_MEMHANDLER_FLAGS__ definition in the makefile and using MEM_TRY_AGAIN like the original OS3 code would cause a failure because the system no longer sets the MEMHF_RECYCLE flag. That fact should be in the AddMemHandler() autodoc.
Someone must have read the include if the makefile is using __OBSOLETE_MEMHANDLER_FLAGS__ and didn't care about it (but mention it in the autodoc couldn't hurt).
xenic wrote:How hard could it be to change exec to treat a MEM_TRY_AGAIN return value the same as a MEM_ALL_DONE return value or MEM_DID_NOTHING return value and solve a potential freeze. I've done all I can to make people aware of the problem.
That would just completly disable the ability for a low memory handler to free some memory in multiple steps. I don't think thats worth it because some old programs might have a buggy lowmemhandler.
xenic
Posts: 1185
Joined: Sun Jun 19, 2011 12:06 am

Re: Serious low memory handler bug (freeze)

Post by xenic »

@salass00 & gazelle
I can see that this is a waste of time and am not going to attempt to explain the issue any more. Apparently, the original Amiga developers were dumb and put the MEMHF_RECYCLE flag and the MEM_TRY_AGAIN return value into the OS for no reason and that the PHD author and experienced programmers that wrote DirectoryOpus5 were idiots for using those unnecessary items.
AmigaOne X1000 with 2GB memory - OS4.1 FE
User avatar
gazelle
Posts: 102
Joined: Sun Mar 04, 2012 12:49 pm
Location: Frohnleiten, Austria

Re: Serious low memory handler bug (freeze)

Post by gazelle »

@xenic:

I don't think you wasted your time. You found a bug and reported it.

After reading about it, it's just my opinion that the bug is in the low memory handler which solely rely on the MEMHF_RECYCLE flag.

Why this flag was removed I don't know. How to solve this for old software, which rely solely on that flag, I also don't know. But just changing exec to treat a MEM_TRY_AGAIN return value the same as a MEM_ALL_DONE is not the solution.
Post Reply