bugCERTI - Bugs: bug #32993, Incorrect use of std::vector

 
 

bug #32993: Incorrect use of std::vector

Submitted by:  Eugene Chemeritskiy <tyz>
Submitted on:  Mon 04 Apr 2011 12:01:44 PM UTC  
 
Category: CERTISeverity: 3 - Normal
Item Group: Software errorStatus: Ready For Test
Privacy: PublicAssigned to: Eric NOULARD <erk>
Open/Closed: ClosedRelease: CVS
Reproducibility: NoneFixed Release: 3.4.1

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Wed 08 Jun 2011 02:44:54 PM UTC, comment #8:

Just checked-in your lastest patch.

Eric NOULARD <erk>
Project AdministratorIn charge of this item.
Wed 01 Jun 2011 05:50:59 PM UTC, comment #7:

>> In fact the design of the RTI API makes it
>> essentially non-reentrant so there wouldn't currently
>> be much benefit to enable thread safety.


> If there is no benefits in internal RTI mutex, there are really
> no reasons to integrate it =). Still it would be great to see
> this information somewhere in the project documentation.


Thas was a little bit black/white judgement of mine.
Some services may be called several time at once
(or in a re-entrant way) but some may not.

The main troubles mainly comes from the callback/tick/evoke
mechanism.
Imagine two threads of the same process are calling two differents
services like SubscribeInteractionClass and
QueryAttributeOwnership then normally both
thread will go to tick/evoke and wait for the corresponding
callback.
How can we ensure that the appropriate thread will get
the appropriate callback during its tick ?

You either need some kind of serialization or accepting
that each thread is a known [unique] LP to the RTI.

More trouble comes if the two threads are trying to
call mutually exclusive services like TAR and NERA for example.

HLA 1.3 document and IEEE-1516-2000 do not even mention
concurrency/reentrance.

The latest IEEE-1516-2010 does (ยง12.6 Concurrency and reentrancy)
here is an excerpt:

_The following requirements apply to an RTI:

  • The RTI shall not make concurrent RTI-initiated service invocations to the same FederateAmbassador instance.
  • The RTI shall support concurrent joined federate-initiated service invocations to the same RTIambassador instance. The service invocations shall be processed, in parallel or sequentially.
  • When in EVOKED mode, all RTI-initiated service invocations shall be made using the same thread context that was used in the invocation of the Evoke Callback/Evoke Multiple Callbacks service.
  • The RTI shall support concurrent joined federate-initiated service invocations to different RTIambassador instances. The calls shall be processed, in parallel or sequentially.
  • An RTIambassador instance shall be capable of creating concurrent federation executions.
  • The RTI shall support invocation by a joined federate of a subset of joined federate-initiated services from within the context of an RTI-initiated service invocation on the same joined federate. Invocation of joined federate-initiated services not within this subset shall generate the Call Not Allowed From Within Callback exception. The allowed subset of joined federate-initiated services is all joined federate-initiated services except Connect, Disconnect, Join Federation Execution, Resign Federation Execution, Evoke Callback, and Evoke Multiple Callbacks._

So in the end we *should* (this is not the case today)
support concurrent calls but we may decide to sequentialize those calls. We could use a big single mutex in each libRTI call however since we *must* (this is the case today) support
reentrant call to federate-initiated service from within a callback (RTI-initiated service) we would at least need to use reentrant mutex.

Finally, this is a interesting question but not as easy as it seem in the first place...
This is definitely not in the short-time plan :-(

But as always, patches are welcome :-]

Eric NOULARD <erk>
Project AdministratorIn charge of this item.
Wed 01 Jun 2011 12:08:37 PM UTC, comment #6:

Hello Eric,

I asked generally because of the comment I found somewhere inside of CERTI sources:
// FIXME This is not thread safe

In my implementation I use several threads inside of one joined federate to simulate the behavior of several logical processes simultaneously. This threads compete with each other and with external federates to access the federation shared objects.

In this scheme data declaration management and ownership management services are used by multiple threads, while the rest of RTI services are invoked in the consolidated way.

After I saw that comment, I realized one of the most foolish mistake I had made in my implementation =). So I had to cope with thread safety issue inside the wrapper over the provided HLA interface.

>> In fact the design of the RTI API makes it
>> essentially non-reentrant so there wouldn't currently
>> be much benefit to enable thread safety.


If there is no benefits in internal RTI mutex, there are really no reasons to integrate it =). Still it would be great to see this information somewhere in the project documentation.

Thank you for your explanations.

Eugene Chemeritskiy <tyz>
Project Member
Tue 31 May 2011 03:44:58 PM UTC, comment #5:

Hi Eugene,

I am in a busy loop but I'll try to
test & merge your patch proposal.

There is no current plan for thread-safety.
In fact the design of the RTI API makes it
essentially non-reentrant so there wouldn't currently
be much benefit to enable thread safety.

i.e. we may introduce a big libRTI mutex but
this wouldn't be very useful.

Why would you require the libRTI to be thread do you
intend to have 1 thread == 1 federate?

i.e. would several threads of the same process call
be joining the federation?

What would be the use case with thread safety requirement ?

Eric NOULARD <erk>
Project AdministratorIn charge of this item.
Fri 27 May 2011 08:30:33 AM UTC, comment #4:

Eric,

I revealed some mistakes in the head ObjectClass.cc version.
It gives up using the counters and changes them to std::vector equivalent. But some conditions still use the old and uninitialized counters.

I've made a fix and attached some sources to check the ownership management services.

BTW, do you have some plans to add thread safety to CERTI?
It was a nasty surprise to find out that the library is not thread safe.

(file #23454, file #23455)

Eugene Chemeritskiy <tyz>
Project Member
Mon 11 Apr 2011 02:44:07 PM UTC, comment #3:

Eric,

I have just checked your fix. It seems to work fine.
I look trough the code. You have done a great job.

As for ping-pong update, I have another test for this. But it depends from some external sources. As I know this contradicts the your test suit policy. I could cut them off but I definitely will not do it before the end of the week.

Eugene Chemeritskiy <tyz>
Project Member
Mon 11 Apr 2011 11:24:46 AM UTC, comment #2:

Just commited a fix to HEAD,

I have no test for this, could you try on your
side and tell me how it works for you.
or may be you can give me an update patch for
your ping-pong test which may use this
https://savannah.nongnu.org/patch/?7487

I tried to do something little more than an hot fix,
should be relatively clean.

Eric NOULARD <erk>
Project AdministratorIn charge of this item.
Mon 04 Apr 2011 01:50:15 PM UTC, comment #1:

Hi,

Investigating that one now.
I think you may be right concerning the CDiffusion class.

I'll read this carefully and see what I can do in order
to make this look better.

The fact is this is only used by ownership management
which hasn't been used so much since a while...

Eric NOULARD <erk>
Project AdministratorIn charge of this item.
Mon 04 Apr 2011 12:01:44 PM UTC, original submission:

Hello,

I found a plenty of similar errors considering incorrect use of std::vector container in libCERTI/ObjectClass.cc.
Elements are inserted into vector without any memory preallocation, and this results in segfault.
Here is an example:

CDiffusion *diffusionAcquisition = new CDiffusion();
..............
diffusionAcquisition.DiffArray[compteur_acquisition].federate = NewOwner ;
diffusionAcquisition.DiffArray[compteur_acquisition].attribute = oa->getHandle();
compteur_acquisition++ ;

By the way, all the founded cases are considered with use of class CDiffusion.
Its purpose is seemed to just wrap std::vector container.
IMHO, this is strange wrapper:
-it has a size field that seems to be useless;
-std::vector - is the only attribute besides the size;
May be the replacement of this structure with analogous typedef is reasonable?

The proposed patch is a merely hotfix.
This file seems to require a great refinements.

Currently I have a theory that this structure originally used to wrap usual array.
So it has a size field. Then the array was replaced by more convenient vector.
But the memory allocation was not taken into account.

Previously I have found the errors considering the relatively
new RTI1516 wrappers, and the deepness of this one is confused me.
The error that lead me to this code is seemed to appear in case of
concurrent attribute ownership acquisition requests.
May be this situation appears really seldom.

Also I found one more error considering the same source file,
but it seems to be better to create one more bug report.

Eugene Chemeritskiy <tyz>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #23455:  newObjectClass.patch added by tyz (1KiB - application/octet-stream)
file #23454:  adoptive_runtime_environment.tar.gz added by tyz (126KiB - application/x-gzip)
file #23098:  ObjectClass.patch added by tyz (9KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by erk (Posted a comment)
  • -unavailable- added by tyz (Submitted the item)
  •  

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 18 Oct 2011 08:37:16 PM UTCerkOpen/ClosedOpen=>Closed
      Fixed ReleaseCVS=>3.4.1
    Wed 08 Jun 2011 02:44:54 PM UTCerkStatusIn Progress=>Ready For Test
    Tue 31 May 2011 03:44:58 PM UTCerkStatusReady For Test=>In Progress
    Fri 27 May 2011 08:30:33 AM UTCtyzAttached File-=>Added newObjectClass.patch, #23455
    Fri 27 May 2011 08:30:32 AM UTCtyzAttached File-=>Added adoptive_runtime_environment.tar.gz, #23454
    Mon 11 Apr 2011 11:24:46 AM UTCerkStatusIn Progress=>Ready For Test
      Fixed ReleaseUnknown=>CVS
    Mon 04 Apr 2011 01:50:15 PM UTCerkStatusNone=>In Progress
      Assigned toNone=>erk
    Mon 04 Apr 2011 12:01:44 PM UTCtyzAttached File-=>Added ObjectClass.patch, #23098

    Back to the top


    Powered by Savane 3.1-cleanup1