[Openocd-development] [PATCH] Fix for segfault in handle_nand_dump_command

[Paul Richards]

Hi,

I found and fixed a segfault in handle_nand_dump_command.

fileio_size() was being called after nand_fileio_finish() which which 
resets the state of the NAND file I/O structure state, including 
NULL-ifying the pointer subsequently accessed by fileio_size().


Cheers,

Paul

From 405d96d7d7acb96b0ef9b4b59afd24929671860e Mon Sep 17 00:00:00 2001
From: Paul Richards 
Date: Wed, 15 Dec 2010 21:42:03 +0900
Subject: [PATCH] Fix for segfault in handle_nand_dump_command.

---
 src/flash/nand/tcl.c |   10 +-
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/flash/nand/tcl.c b/src/flash/nand/tcl.c
index 15cf179..af91fc8 100644
--- a/src/flash/nand/tcl.c
+++ b/src/flash/nand/tcl.c
@@ -357,6 +357,7 @@ COMMAND_HANDLER(handle_nand_verify_command)
 
 COMMAND_HANDLER(handle_nand_dump_command)
 {
+   int filesize;
struct nand_device *nand = NULL;
struct nand_fileio_state s;
int retval = CALL_COMMAND_HANDLER(nand_fileio_parse_args,
@@ -386,13 +387,12 @@ COMMAND_HANDLER(handle_nand_dump_command)
s.address += nand->page_size;
}
 
+   retval = fileio_size(&s.fileio, &filesize);
+   if (retval != ERROR_OK)
+   return retval;
+
if (nand_fileio_finish(&s) == ERROR_OK)
{
-   int filesize;
-   retval = fileio_size(&s.fileio, &filesize);
-   if (retval != ERROR_OK)
-   return retval;
-
command_print(CMD_CTX, "dumped %ld bytes in %fs (%0.3f KiB/s)",
(long)filesize, duration_elapsed(&s.bench),
duration_kbps(&s.bench, filesize));
-- 
1.7.2.3

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development

Re: [Openocd-development] Marvell Armada Support

[Paul Richards]

Hi Enrico,

On 2010/12/05 20:41, Enrico Scholz wrote:

Base operations are already possible by



Thanks for this, sorry I missed your mail first time around.


see [1] for full configuration.  The trick is the first dummy TAP which
is mentioned in some errata entry (an extra TDO is shifted in concatening
mode).


It looks like we came to the same conclusions, there's very limited 
information still.



I wrote a NAND driver for the PXA168 (see [2]; PXA320 should be very
similar btw) but it is very slow and although I intended to keep it as
general as possible, I ended with hardcoding certain values of the used
NAND chip.  Bad block management is missing too.


Thanks, very helpful and perfect timing.  The NAND is next for me so 
I'll give the driver a try and let you know how I go.


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]

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()

[Paul Richards]

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]

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()

[Paul Richards]

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()

[Paul Richards]

I'm attaching the patch proposed in the last mail as I'll be offline 
shortly.


Regards,

Paul

On 2010/12/08 18:02, Paul Richards wrote:

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 K&R bracing, though most of the other 
code in this file doesn't.  Is there a preference?


Thanks,

Paul



From 987ecc00efaf0ee4dd2fae7b68bf4cf5a4272902 Mon Sep 17 00:00:00 2001
From: Paul Richards 
Date: Wed, 8 Dec 2010 15:48:55 +0900
Subject: [PATCH] Fix for segmentation fault from freed memory access in 
jtag_unregister_event_callback()

---
 src/jtag/core.c |   22 +-
 1 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/jtag/core.c b/src/jtag/core.c
index c1b64bb..f3408e1 100644
--- a/src/jtag/core.c
+++ b/src/jtag/core.c
@@ -296,28 +296,24 @@ 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 **callbacks_p;
-   struct jtag_event_callback **next;
+   struct jtag_event_callback **p = &jtag_event_callbacks, *temp;
 
if (callback == NULL)
{
return ERROR_INVALID_ARGUMENTS;
}
 
-   for (callbacks_p = &jtag_event_callbacks;
-   *callbacks_p != NULL;
-   callbacks_p = next)
+   while (*p)
{
-   next = &((*callbacks_p)->next);
-
-   if ((*callbacks_p)->priv != priv)
-   continue;
-
-   if ((*callbacks_p)->callback == callback)
+   if (((*p)->priv != priv) || ((*p)->callback != callback))
{
-   free(*callbacks_p);
-   *callbacks_p = *next;
+   p = &(*p)->next;
+   continue;
}
+
+   temp = *p;
+   *p = (*p)->next;
+   free(temp);
}
 
return ERROR_OK;
-- 
1.7.2.3

___
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]

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 K&R 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

Re: [Openocd-development] Bug in core.c::jtag_unregister_event_callback()

[Paul Richards]

Hi Peter,

I just saw your mail after submitting a patch.  I kept the double 
pointer usage for consistency but simplified the logic.


On 2010/12/08 15:35, Peter Stuge wrote:


Should it? I don't think the following callback should be skipped
just because the current one is removed.


I believe you're correct, must have been getting late.  My patch doesn't 
advance the pointer and iterates once again with the updated value.


Regards,

Paul

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development

[Openocd-development] [PATCH] Fix for segmentation fault from freed memory access in jtag_unregister_event_callback()

[Paul Richards]

Fix for issue described in the following mail

https://lists.berlios.de/pipermail/openocd-development/2010-December/017479.html

From c78f5203622438e73580c327a6e55a0df17b85a8 Mon Sep 17 00:00:00 2001
From: Paul Richards 
Date: Wed, 8 Dec 2010 15:48:55 +0900
Subject: [PATCH] Fix for segmentation fault from freed memory access in 
jtag_unregister_event_callback()

---
 src/jtag/core.c |   22 ++
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/src/jtag/core.c b/src/jtag/core.c
index c1b64bb..4f75e1c 100644
--- a/src/jtag/core.c
+++ b/src/jtag/core.c
@@ -296,27 +296,25 @@ 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 **callbacks_p;
-   struct jtag_event_callback **next;
+   struct jtag_event_callback **callbacks_p = &jtag_event_callbacks;
 
if (callback == NULL)
{
return ERROR_INVALID_ARGUMENTS;
}
 
-   for (callbacks_p = &jtag_event_callbacks;
-   *callbacks_p != NULL;
-   callbacks_p = next)
+   while (*callbacks_p != NULL)
{
-   next = &((*callbacks_p)->next);
-
-   if ((*callbacks_p)->priv != priv)
-   continue;
+   if (((*callbacks_p)->priv != priv) && ((*callbacks_p)->callback 
== callback))
+   {
+   struct jtag_event_callback *free_callback = 
*callbacks_p;
 
-   if ((*callbacks_p)->callback == callback)
+   *callbacks_p = (*callbacks_p)->next;
+   free(free_callback);
+   }
+   else
{
-   free(*callbacks_p);
-   *callbacks_p = *next;
+   callbacks_p = &((*callbacks_p)->next);
}
}
 
-- 
1.7.2.3

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development

Re: [Openocd-development] Bug in core.c::jtag_unregister_event_callback()

[Paul Richards]

Hi Øyvind

On 2010/12/08 3:58, Øyvind Harboe wrote:

Looks like bugs indeed...

How did you come across them?


The processor I am using (PXA16x) has TAPs (including the CPU TAP 
controller) that must be switched in via the default TAP.  When I 
enabled the CPU TAP post-init the enable event callback was being 
unregistered and this was generating a segmentation fault.


Thanks for the submission information, I'll submit shortly.  The changes 
are minor, but please let me know if there's anything I missed with the 
coding style, etc.


Regards,

Paul

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development

[Openocd-development] Bug in core.c::jtag_unregister_event_callback()

[Paul Richards]

Hi,

I believe I've found a bug or two in jtag_unregister_event_callback().

273 
 
int jtag_register_event_callback(jtag_event_handler_t callback, void *priv)
274 
 
{

...
311 
 
next = &((*callbacks_p)->next);

...
316 
 
if ((*callbacks_p)->callback == callback)
317 
 
{
318 
 
free(*callbacks_p);
319 
 
*callbacks_p = *next;
320 
 
}


The assignment after the free() call is accessing the callback structure 
that has just been freed.  Also, the assignment on line 319 doesn't 
account for the next increment in the for statement.


307 
 
for (callbacks_p = &jtag_event_callbacks;
308 
 
*callbacks_p != NULL;
309 
 
callbacks_p = next)


I have fixed and verified on my build but am unfamiliar with submission 
procedures.  I'm happy to contribute with a little guidance.


Regards,

Paul

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development

[Openocd-development] Marvell Armada Support

[Paul Richards]

Hello,

Are there any plans to add support for the Marvell Armada processor 
family to Open OCD?  (A hybrid of Feroceon and XScale processors from 
what I understand).


I've had a little success customising scripts to communicate with an 
Armada PXA168 target via a FT4232 interface, though I believe code 
additions will be required.  Although new to OpenOCD I'm willing to 
contribute what I can if no similar activities are already underway.


Regards,

Paul

___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development