Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
On Fri, Dec 10, 2010 at 9:28 AM, Peter Stuge pe...@stuge.se wrote: Øyvind Harboe wrote: Two versions attached. I'll leave it to you to decide which is the best way to implement this I prefer the longer but simpler to read one. Were it mostly a smaller my own project then I'd gone with the shorter one. I think optimizing for testability readability is the way to go here. Especially considering the evidence that this code doesn't get tested a whole lot -- Øyvind Harboe Can Zylin Consulting help on your project? US toll free 1-866-980-3434 / International +47 51 63 25 00 http://www.zylin.com/zy1000.html ARM7 ARM9 ARM11 XScale Cortex JTAG debugger and flash programmer ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
On 10/12/2010 08:34, Øyvind Harboe wrote: On Fri, Dec 10, 2010 at 9:28 AM, Peter Stugepe...@stuge.se wrote: Øyvind Harboe wrote: Two versions attached. I'll leave it to you to decide which is the best way to implement this I prefer the longer but simpler to read one. Were it mostly a smaller my own project then I'd gone with the shorter one. I think optimizing for testability readability is the way to go here. Especially considering the evidence that this code doesn't get tested a whole lot Not looked into it but why do we not just duplicate the existing unregister event/timer functions - or are they broken aswell? Cheers Spen ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
On 2010/12/10 17:34, Øyvind Harboe wrote: I think optimizing for testability readability is the way to go here. Especially considering the evidence that this code doesn't get tested a whole lot I can verify that both solutions work. I'm OK with whichever solution you guys choose since you've been around here a lot longer than I :-) Cheers, Paul ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
On 2010/12/10 18:46, Spencer Oliver wrote: Not looked into it but why do we not just duplicate the existing unregister event/timer functions - or are they broken aswell? They (the target versions) don't appear to have the same problem. The only difference I can see is that the jtag_unregister_event_callback() continues to look for further events to unregister. I'm not familiar enough with the code to know why that might be possible. Easy enough to continue iterating if it is required. Now there's 3 versions to choose from :-) I would have copied these had I known they were there. Cheers, Paul ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
On Fri, Dec 10, 2010 at 4:16 PM, Paul Richards paulr...@gmail.com wrote: On 2010/12/10 18:46, Spencer Oliver wrote: Not looked into it but why do we not just duplicate the existing unregister event/timer functions - or are they broken aswell? They (the target versions) don't appear to have the same problem. The only difference I can see is that the jtag_unregister_event_callback() continues to look for further events to unregister. I'm not familiar enough with the code to know why that might be possible. Easy enough to continue iterating if it is required. Now there's 3 versions to choose from :-) I would have copied these had I known they were there. It's not too late. The target versions seems nice and readable. And correct, as far as I can see this late hour. I think the jtag version should follow the behavior of these and only remove the first matching handler. That would be the correct thing to do, if there was a point in having the same handler registered more than once. The only issue I have with copying the target version is that having the same code duplicated in three places probably warrants refactoring it into a helper function instead. Regards, Andreas ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
Andreas Fritiofson wrote: Now there's 3 versions to choose from :-) It's not too late. The target versions seems nice and readable. And correct, as far as I can see this late hour. I think the jtag version should follow the behavior of these and only remove the first matching handler. That would be the correct thing to do, if there was a point in having the same handler registered more than once. Really? How are the callbacks being used? The only issue I have with copying the target version is that having the same code duplicated in three places probably warrants refactoring it into a helper function instead. Which is difficult for something simple as a linked list, unless the list entries are identical, and of the same type. Are they? //Peter ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
On Fri, Dec 10, 2010 at 11:39 PM, Peter Stuge pe...@stuge.se wrote: Andreas Fritiofson wrote: Now there's 3 versions to choose from :-) It's not too late. The target versions seems nice and readable. And correct, as far as I can see this late hour. I think the jtag version should follow the behavior of these and only remove the first matching handler. That would be the correct thing to do, if there was a point in having the same handler registered more than once. Really? How are the callbacks being used? I don't know. But, generally, if I _can_ register the same handler twice, I'd expect each registration to require a separate unregister call. The actual call sites would have to be inspected before changing this behavior, of course. There shouldn't be many of them, since the bug has gone unnoticed for so long. The only issue I have with copying the target version is that having the same code duplicated in three places probably warrants refactoring it into a helper function instead. Which is difficult for something simple as a linked list, unless the list entries are identical, and of the same type. Are they? Two of them differ only in the callback signature, the third is very different. There are several imaginable solutions. You could have the list functions operate on a generic next pointer, included as the first element of all specialized list types. Or the actual data could be stored outside the list structure, with just a void pointer in it. Both require some nasty casts. Neither is likely to be more clear and concise than the duplicated code. Maybe if it was used in _ten_ places... /Andreas ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
On 2010/12/11 8:20, Andreas Fritiofson wrote: I don't know. But, generally, if I _can_ register the same handler twice, I'd expect each registration to require a separate unregister call. The actual call sites would have to be inspected before changing this behavior, of course. There shouldn't be many of them, since the bug has gone unnoticed for so long. Looking at this further, there are only two places that jtag_register_event_callback() is called from, the first from jtag_tap_init() with the a new TAP instance as the priv parameter, and the second from target_examine() with a target as the priv parameter. The first case I believe can only be called once per unique priv instance by virtue of the fact that it's part of the TAP initialisation and the TAP instance is the priv parameter. The second case only appears to be called from the init handler which is guarded by a static initialized flag. However, there is nothing to help prevent issues being introduced with their future usage. At the very least the functions should have banner describing the interface contract, specifically whether multiple registration are disallowed, allowed with balanced de-registration, or allowed with only a single de-registration necessary. I think the former with safety checks would be the safest, however this would increase the footprint of change if we were to require consistency. For now I suggest we can stay consistent with the target based list function implementations? This would appear to imply that multiple registrations are possible but registration / de-registration balance is the responsibility of the client. I also think the benefits of a common list function here are outweighed by the simplicity of the function (the bug and the generated discussion excepted!) and lost readability in the specialisation casts. I'd be happy to test and submit a new patch candidate if and when we're all in agreement. Peter, note that the double pointers are in the target list function implementations (that would be copied), are you ok with this? I think the implementations are concise and readable, though maybe my sensitivity to double pointers has dulled... Cheers, Paul ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
Paul Richards wrote: Peter, note that the double pointers are in the target list function implementations (that would be copied), are you ok with this? Afraid not. The alternative is quite straightforward, so I would strongly prefer not going there. Thanks! //Peter ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
On 2010/12/11 13:00, Peter Stuge wrote: Afraid not. The alternative is quite straightforward, so I would strongly prefer not going there. Thought as much :-) Well, will leave it up to you guys to decide. I can test any new patches required. Cheers, Paul ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
Peter Stuge wrote: The alternative is quite straightforward, so I would strongly prefer not going there. Allow me to clarify that; The alternative of rewriting to use no double pointers makes the code much more straightforward, so I would strongly prefer not using any double pointers. :) //Peter ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
Merged. Thanks! -- Øyvind Harboe Can Zylin Consulting help on your project? US toll free 1-866-980-3434 / International +47 51 63 25 00 http://www.zylin.com/zy1000.html ARM7 ARM9 ARM11 XScale Cortex JTAG debugger and flash programmer ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
Øyvind Harboe wrote: Merged. So I should have complained more loudly. The double pointers are horrible. //Peter ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
On Fri, Dec 10, 2010 at 8:05 AM, Peter Stuge pe...@stuge.se wrote: Øyvind Harboe wrote: Merged. So I should have complained more loudly. The double pointers are horrible. Ah, missed that. Well another patch is gladly accepted, meanwhile it no longer crashes. Hopefully the fact that I accepted a patch, won't completely kill any motivation to do some more cleanup. I must admit I had to read over it a couple of times, but I didn't want to touch the code as I didn't have a way to readily reproduce/test it. -- Øyvind Harboe Can Zylin Consulting help on your project? US toll free 1-866-980-3434 / International +47 51 63 25 00 http://www.zylin.com/zy1000.html ARM7 ARM9 ARM11 XScale Cortex JTAG debugger and flash programmer ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
Øyvind Harboe wrote:
double pointers are horrible.
Ah, missed that. Well another patch is gladly accepted, meanwhile
it no longer crashes.
Fair. Two versions attached. One which makes the code two lines
longer and avoids double pointers, and a remix which uses only a
single double pointer and is net one line shorter.
//Peter
From 2794909b0654630d71fe0fd618d7188a450234c4 Mon Sep 17 00:00:00 2001
From: Peter Stuge pe...@stuge.se
Date: Fri, 10 Dec 2010 08:19:23 +0100
Subject: [PATCH] jtag: Avoid double pointers when removing event callbacks
---
src/jtag/core.c | 22 --
1 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/src/jtag/core.c b/src/jtag/core.c
index dfedc17..dd6c8d0 100644
--- a/src/jtag/core.c
+++ b/src/jtag/core.c
@@ -296,24 +296,26 @@ int jtag_register_event_callback(jtag_event_handler_t
callback, void *priv)
int jtag_unregister_event_callback(jtag_event_handler_t callback, void *priv)
{
- struct jtag_event_callback **p = jtag_event_callbacks, *temp;
+ struct jtag_event_callback *p, *prev, *next;
- if (callback == NULL)
- {
+ if (!callback)
return ERROR_INVALID_ARGUMENTS;
- }
- while (*p)
+ for (p = jtag_event_callbacks, prev = NULL, p, p = next)
{
- if (((*p)-priv != priv) || ((*p)-callback != callback))
+ next = p-next;
+
+ if (p-callback != callback || p-priv != priv)
{
- p = (*p)-next;
+ prev = p;
continue;
}
- temp = *p;
- *p = (*p)-next;
- free(temp);
+ if (prev)
+ prev-next = next;
+ else
+ jtag_event_callbacks = next;
+ free(p);
}
return ERROR_OK;
--
1.7.2
From 1063bf4e9dcb722f39f015af47864b3583e6bdb1 Mon Sep 17 00:00:00 2001
From: Peter Stuge pe...@stuge.se
Date: Fri, 10 Dec 2010 08:19:23 +0100
Subject: [PATCH] jtag: Use only one double pointer when removing event callbacks
---
src/jtag/core.c | 19 +--
1 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/src/jtag/core.c b/src/jtag/core.c
index dfedc17..6a97079 100644
--- a/src/jtag/core.c
+++ b/src/jtag/core.c
@@ -296,24 +296,23 @@ int jtag_register_event_callback(jtag_event_handler_t
callback, void *priv)
int jtag_unregister_event_callback(jtag_event_handler_t callback, void *priv)
{
- struct jtag_event_callback **p = jtag_event_callbacks, *temp;
+ struct jtag_event_callback *p, *prev, *next;
- if (callback == NULL)
- {
+ if (!callback)
return ERROR_INVALID_ARGUMENTS;
- }
- while (*p)
+ for (p = jtag_event_callbacks, prev = NULL, p, p = next)
{
- if (((*p)-priv != priv) || ((*p)-callback != callback))
+ next = p-next;
+
+ if (p-callback != callback || p-priv != priv)
{
- p = (*p)-next;
+ prev = p;
continue;
}
- temp = *p;
- *p = (*p)-next;
- free(temp);
+ *(prev ? prev-next : jtag_event_callbacks) = next;
+ free(p);
}
return ERROR_OK;
--
1.7.2
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
On Fri, Dec 10, 2010 at 8:27 AM, Peter Stuge pe...@stuge.se wrote: Øyvind Harboe wrote: double pointers are horrible. Ah, missed that. Well another patch is gladly accepted, meanwhile it no longer crashes. Fair. Two versions attached. One which makes the code two lines longer and avoids double pointers, and a remix which uses only a single double pointer and is net one line shorter. I'll leave it to you to decide which is the best way to implement this and to incorporate any feedback. Give the word when it is ready to be committed and which patch is the winner. -- Øyvind Harboe Can Zylin Consulting help on your project? US toll free 1-866-980-3434 / International +47 51 63 25 00 http://www.zylin.com/zy1000.html ARM7 ARM9 ARM11 XScale Cortex JTAG debugger and flash programmer ___ Openocd-development mailing list Openocd-development@lists.berlios.de https://lists.berlios.de/mailman/listinfo/openocd-development
Re: [Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()
On 2010/12/08 16:27, Peter Stuge wrote:
The priv comparison logic is reversed.
Sorry, can't believe that snuck in, thanks.
The code is horrible. (Certainly not your fault!) Sorry if that's
offensive to the original author. The variable names do not help.
Will this work:
int jtag_unregister_event_callback(callback,priv) {
struct jtag_event_callback *p, *prev = NULL, *next;
if (!callback)
return ERROR_INVALID_ARGUMENTS;
for (p = jtag_event_callbacks; p; p = next) {
next = p-next;
if (p-priv != priv || p-callback != callback) {
prev = p;
continue;
}
if (prev)
prev-next = next;
else
jtag_events_callbacks = next;
free(p);
}
}
How about the following, it cleans up the naming, but does keep the
double pointer usage which I think leaves it more compact, but debatably
harder to read?
int jtag_unregister_event_callback(jtag_event_handler_t callback, void
*priv)
{
struct jtag_event_callback **p = jtag_event_callbacks, *temp;
if (callback == NULL)
{
return ERROR_INVALID_ARGUMENTS;
}
while (*p)
{
if (((*p)-priv != priv) || ((*p)-callback != callback))
{
p = (*p)-next;
continue;
}
temp = *p;
*p = (*p)-next;
free(temp);
}
return ERROR_OK;
}
I'm happy to remove the the double pointer if you prefer, I personally
prefer the elegance in the compactness, but I'm happy to go along with
what the pereferences are here.
Also I note that you use the KR bracing, though most of the other code
in this file doesn't. Is there a preference?
Thanks,
Paul
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
