Locked task, returning references

[Max Kellermann]

Hi John,

please have a look at the following method in Task.cpp:

 const GEOPOINT &
 TaskSafe::getActiveLocation()  
 { // read
   Poco::ScopedRWLock protect(lock, false);
   return _task.getActiveLocation();
 }

Here, you are calling Task::getActiveLocation() while it is locked -
correct so far.  But the pointer to the GEOPOINT object leaves the
scope of the lock; any further access which dereferences this pointer
is illegal, because the task isn't locked anymore.

The same is true for any other method which returns a
pointer/reference.  To be safe, you must return by value at this
point.  Or make the caller hold the lock while he's holding the
reference.

Max

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Xcsoar-devel mailing list
Xcsoar-devel@...
https://lists.sourceforge.net/lists/listinfo/xcsoar-devel

[Max Kellermann]

On 2009/09/24 11:41, Max Kellermann <max@...> wrote:
> please have a look at the following method in Task.cpp:

There are a more types of race conditions, e.g. in
GlideComputerTask::AATStats_Distance():

  if(task.Valid()) {
    i=task.getActiveIndex();
    // ...
      LegToGo = Distance(Basic().Location,
                         task.getTaskPointLocation(i));

Each TaskSafe method call is indeed safe, but only for itself.
Between two TaskSafe method calls, the task may have changed, and all
information you might have got from it might have become invalid.

Example: task.getActiveIndex() returns an integer which *was* valid
before TaskSafe::getActiveIndex() returned (until the lock was
released).  At any time between TaskSafe::getActiveIndex() and
TaskSafe::getTaskPointLocation(), "i" might become invalid, and may
crash XCSoar.

Nearly all calls to TaskSafe are racy, voiding any positive effect of
the whole TaskSafe class.  This needs a serious overhaul.  Remember
the concept I proposed before you implemented all this?

Max

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Xcsoar-devel mailing list
Xcsoar-devel@...
https://lists.sourceforge.net/lists/listinfo/xcsoar-devel

[John Wharington-2]

Hi Max,

You're right.  It should return a copy.  My bad.

On Thu, 2009-09-24 at 11:41 +0200, Max Kellermann wrote:

> Hi John,
>
> please have a look at the following method in Task.cpp:
>
>  const GEOPOINT &
>  TaskSafe::getActiveLocation()  
>  { // read
>    Poco::ScopedRWLock protect(lock, false);
>    return _task.getActiveLocation();
>  }
>
> Here, you are calling Task::getActiveLocation() while it is locked -
> correct so far.  But the pointer to the GEOPOINT object leaves the
> scope of the lock; any further access which dereferences this pointer
> is illegal, because the task isn't locked anymore.
>
> The same is true for any other method which returns a
> pointer/reference.  To be safe, you must return by value at this
> point.  Or make the caller hold the lock while he's holding the
> reference.
>
> Max

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Xcsoar-devel mailing list
Xcsoar-devel@...
https://lists.sourceforge.net/lists/listinfo/xcsoar-devel

[John Wharington-2]

Don't worry about this for now as the new task system will more
effectively be separated out.

I do remember the concept you proposed.

The TaskSafe stuff while not perfect does make inroads but isn't
finished and most likely the new task system will have a different
mechanism to provide access to the task.

I am not actively working on the master branch of XCSoar as I am busy
working on the task system.


On Thu, 2009-09-24 at 11:52 +0200, Max Kellermann wrote:

> On 2009/09/24 11:41, Max Kellermann <max@...> wrote:
> > please have a look at the following method in Task.cpp:
>
> There are a more types of race conditions, e.g. in
> GlideComputerTask::AATStats_Distance():
>
>   if(task.Valid()) {
>     i=task.getActiveIndex();
>     // ...
>       LegToGo = Distance(Basic().Location,
>                          task.getTaskPointLocation(i));
>
> Each TaskSafe method call is indeed safe, but only for itself.
> Between two TaskSafe method calls, the task may have changed, and all
> information you might have got from it might have become invalid.
>
> Example: task.getActiveIndex() returns an integer which *was* valid
> before TaskSafe::getActiveIndex() returned (until the lock was
> released).  At any time between TaskSafe::getActiveIndex() and
> TaskSafe::getTaskPointLocation(), "i" might become invalid, and may
> crash XCSoar.
>
> Nearly all calls to TaskSafe are racy, voiding any positive effect of
> the whole TaskSafe class.  This needs a serious overhaul.  Remember
> the concept I proposed before you implemented all this?
>
> Max

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
Xcsoar-devel mailing list
Xcsoar-devel@...
https://lists.sourceforge.net/lists/listinfo/xcsoar-devel