Login
Username:

Password:

Remember me



Lost Password?

Register now!

Sections

Who's Online
71 user(s) are online (36 user(s) are browsing Forums)

Members: 1
Guests: 70

FlynnTheAvatar, more...

Headlines

 
  Register To Post  

« 1 (2)
Re: let's deal with remaining bugs in dopus5 together
Just popping in
Just popping in


See User information
Quote:

LiveForIt wrote:
@Karlos
Is it dangerous?


Without question.

Quote:
Only if the list is shared, we don't know that from the what is posted here.


You don't need to know whether the list is shared or not to understand that referencing already freed memory is a big mistake. Look at the original code: there is a rock-solid reason that the next pointer is assigned the value of ln_Succ *before* the call to L_FreeFiletype() on the node.

Maybe you'll be lucky most of the time and the ln_Succ member at whatever address/offset the original node existed at a moment ago is still valid but to assume so would be foolhardy at best. Especially when you consider that OS4 is moving away from casual uses of Forbid() to prevent other tasks from pre-empting your code away and implements fast-turnaround slab allocators for structures. The chances that the memory your node was just in a moment ago will be recycled for someone else's use is likely greater than it was in 3.x.


-edit-

Quote:
If its NOT a OS structure it should be protected by Mutex.


I have to ask... really?

Look, Mutexes and Semaphores are there to ensure a resource is properly accessed in a concurrent environment. They are not there to protect you from playing Russian Roulette with pointers.

You have no idea what L_FreeFiletype() is going to do to the memory your node lives in. Probably, it will just free it, but it could do anything. Suppose these nodes are created by an allocator function. For all you know, the code might manage a private pool of them. You have no way of knowing what this function does to the memory occupied by the node. It might re-assign them to some value, resulting in a DSI when you try to dereference the pointer you read back. Or maybe it just zeroes it, in which case, your loop finishes early without processing the rest of the list which results in a memory leak (and perhaps other resources too).


Edited by Karlos on 2013/6/1 20:16:48
Go to top
Re: let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
@Karlos

Quote:

Maybe you'll be lucky most of the time and the ln_Succ member at whatever address/offset the original node existed at a moment ago is still valid but to assume so would be foolhardy at best.


Your assuming that that you have more then one thread or there are many applications, sorting the list, deleing nodes, and adding nodes, while this program is the for loop then yes.

If if assume that this ia isolated list where only this program has control of what happens, then we can assume that address of ln_succ will be untouch after the current node is freed.

Quote:
Look, Mutexes and Semaphores are there to ensure a resource is properly accessed in a concurrent environment.


I agree a resource in the form of a list or node maybe?

Quote:
They are not there to protect you from playing Russian Roulette with pointers.


A pointer can also be a resource your trying to protect.

But its important that is what ever you do you need to make shore its not a some thing that OS expects being protected by Forbid() / Permit(), it should be oblivious way this wont work.

I can understand you want the OS to deal whit it, but then you need a stub not a macro, or else the legacy will just break any improvement.

I don't see how it even possible to implement a sort of obtain / release mechanism based on fact that application is simply asking for the successor node, thats simply assuming too mutch, nor will it work in this code example.

Quote:
You have no idea what L_FreeFiletype()


We need to assume it does what code imply it does, free one node, not a pool of memory. Or else the code is buggy and broken, and some one needs to fix it.


Edited by LiveForIt on 2013/6/2 2:44:38
Edited by LiveForIt on 2013/6/2 8:05:08
(NutsAboutAmiga)

Basilisk II for AmigaOS4
AmigaInputAnywhere
Excalibur
and other tools and apps.
Go to top
Re: let's deal with remaining bugs in dopus5 together
Quite a regular
Quite a regular


See User information
@Karlos:

Karl, I was not Removing the node in the snippet of code that I proposed. I do know better than that!

[edit]
Sorry, I didn't realise that "L_FreeFiletype()" might free the Node in the argument.
If it does, then the "Next" node must be saved before the call, as you so rightly point out.
[/edit]


Edited by tonyw on 2013/6/2 5:06:28
cheers
tony
Go to top
Re: let's deal with remaining bugs in dopus5 together
Just popping in
Just popping in


See User information
Quote:

tonyw wrote:
@Karlos:

Karl, I was not Removing the node in the snippet of code that I proposed. I do know better than that!

[edit]
Sorry, I didn't realise that "L_FreeFiletype()" might free the Node in the argument.
If it does, then the "Next" node must be saved before the call, as you so rightly point out.
[/edit]


The point is that neither of us know what side effects L_FreeFiletype() have. However, my assumption that it frees the node and thus renders it's structure members invalid is based on what I would say is reasonable inference, when you look at the original code:

// Free filetypes in list 
for (node=list->filetype_list.lh_Head;node->ln_Succ;) 

  
next=node->ln_Succ
  
L_FreeFiletype((Cfg_Filetype *)node); 
  
node=next
}


The comment, function name and "get the successor node before" strategy strongly suggests that this is the case.


Edited by Karlos on 2013/6/2 12:16:40
Go to top
Re: let's deal with remaining bugs in dopus5 together
Just popping in
Just popping in


See User information
Quote:

LiveForIt wrote:
@Karlos

We need to assume it does what code imply it does, free one node, not a pool of memory. Or else the code is buggy and broken, and some one needs to fix it.


I didn't say it freed a pool of memory. I said it (probably) freed the node. But it could do anything. The point is, the original code took the ln_Succ node's value *before* calling the L_FreeFiletype() on the node and not *after*. The main reason you do something like that before calling any function is because you know that member isn't going to be valid *after*.

Also, the whole argument about multiple threads and what not is irrelevant. Accessing the properties of any structure that has been freed is flat out wrong, regardless of whether you are in the same thread of execution or a different one.

Quote:
If if assume that this ia isolated list where only this program has control of what happens, then we can assume that address of ln_succ will be untouch after the current node is freed.


Resized Image


You can assume no such thing!

What if L_FreeFiletype() zeroes out the member pointers or assigns them some special "marked free for reallocation" value or anything? What if during the process, the kernel fills the space that was allocated with some special value when it's freed? And yes, there are debug builds which do this sort of thing to help developers identify the specific type of lunacy you are suggesting can be assumed ;)

Bottom line: you have no idea that it's safe to access the node after calling L_FreeFiletype() and the only reasonable thing you can infer from the original code is that it isn't.


Edited by Karlos on 2013/6/2 10:31:52
Go to top
Re: let's deal with remaining bugs in dopus5 together
Just popping in
Just popping in


See User information
I tracked down the implementation of the function in question in the source:

void __asm __saveds L_FreeFiletype(register __a0 Cfg_Filetype *type)
{
        
short a;

        if (!
type) return;

        
// Remove filetype from list
        
if (type->node.ln_Succ && type->node.ln_Pred)
                
Remove(&type->node);

        
// Free data
        
L_FreeMemH(type->recognition);
        
L_FreeMemH(type->icon_path);
        if (
type->actions)
        {
                for (
a=0;a<16;a++)
                        
L_FreeMemH(type->actions[a]);
                
L_FreeMemH(type->actions);
        }
        
config_free_functions(&type->function_list);
        
L_FreeMemH(type);
}


As you can see, Remove() is invoked on the node. And, as I suggested my previous post, the kernel does indeed make changes to the node structure. From the Remove() documentation:

*   NOTES
*        As of V50this function modifies the ln_Succ and ln_Pred
*        pointers to prevent disaster from Removing a node twice.
*   
EXAMPLE
*    \* If traversing 'list' with GetHead/GetSucc and nodes may at
*     * some point be Remove()edit is not permitted to read any of
*     * the node linkage fields after it has left the list, as removing
*     * the node invalidates the list pointersthereforeit is 
*     * necessary to determine the successor before removal.
*     * 
Notewhen using these functionsdon't check the 
*     * currentNode->ln_Succ, just for currentNode != NULL.
*     *\



So yes, it's completely illegal to access ln_Succ *after* calling the function. Whatever value they had previously is overwritten by the call to execsg's Remove().

Go to top
Re: let's deal with remaining bugs in dopus5 together
Home away from home
Home away from home


See User information
@all
Xenic fix both problems (and garbled text, and crash). Garbled text was because of some problems with varargs, and crash was because of how filetype code done.

Anyone who want to help with another bugs, welcome to https://sourceforge.net/p/dopus5allami ... sion/dev/thread/b0616ad4/

In first post all the bugs listed

Join us to improve dopus5!
AmigaOS4 on youtube
Go to top
Re: let's deal with remaining bugs in dopus5 together
Just can't stay away
Just can't stay away


See User information
Excellent progress! Thanks.

AmigaOne X1000.
Radeon RX550

http://www.tinylife.org.uk/
Go to top

  Register To Post
« 1 (2)

 




Currently Active Users Viewing This Thread: 1 ( 0 members and 1 Anonymous Users )




Powered by XOOPS 2.0 © 2001-2023 The XOOPS Project