bugCERTI - Bugs: bug #23921, Crashed on updateAttributeValues

 
 

bug #23921: Crashed on updateAttributeValues

Submitted by:  Zheng Yanjun <zyjtsc>
Submitted on:  Thu 24 Jul 2008 08:56:35 AM UTC  
 
Category: CERTISeverity: 5 - Blocker
Item Group: Software errorStatus: Ready For Test
Privacy: PublicAssigned to: Petr Gotthard <gotthardp>
Open/Closed: ClosedRelease: 3.3.0
Reproducibility: Every TimeFixed Release: 3.3.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)

Fri 19 Sep 2008 01:23:41 PM UTC, comment #11:

I re-assign the bug to Petr since he was faster
at least this time :=)

Eric NOULARD <erk>
Project Administrator
Thu 18 Sep 2008 02:42:56 PM UTC, comment #10:

Fixed.

Petr Gotthard <gotthardp>
Project MemberIn charge of this item.
Thu 18 Sep 2008 10:10:41 AM UTC, comment #9:

You can get rid of typedef which are unused
and not part of external HLA interface
which should be the case for the type
defined inside certi:: namespace like those.

Applications should not use certi:: types,
the Billard does (certi::MessageBuffer)
but the HLATestsSuite and HLA_Tutorial do not.

I suggest not to reuse those type unless it's appropriate.
For example, I think:

typedef char *AttributeValue

should be deleted and replaced with

typedef std::string AttributeValue_t

I'd like to use _t suffix in order
to make it clear it's a type.

If you encounter type which are still in use
(like typedef char *ObjectName ; )
but may be replace with std::string do not
change them with this patch.

We should do it separatly in order not to include
two much changes in a single patch, this would
make it harder to track regression (if any).

Eric NOULARD <erk>
Project Administrator
Thu 18 Sep 2008 09:32:08 AM UTC, comment #8:

Give me few hours. I made the update you suggested, but:

(1) I got a new segmentation fault that needs to be fixed.

(2) I have a question. ;-)
In certi.hh I found some typedefs that are not used and that may be re-used to solve the issue no.1 you mentioned.
Namely:
// Deprecated HLA types still in use
typedef char *AttributeValue ;

// typedef std::vector<char> AttributeValue;
typedef char *ObjectName ;
typedef AttributeValue ParameterValue ;
typedef char *ParameterName ;
typedef AttributeValue ValueName ;

struct Value {
ValueName name ;
ValueName value ;
ValueName type ;
ULong length ;
};

Can the unused typedefs be deleted or re-used, or are there some applications that might use these?

Petr Gotthard <gotthardp>
Project MemberIn charge of this item.
Wed 17 Sep 2008 09:15:56 PM UTC, comment #7:

Petr,

If you don't have time to update your patch,
apply-it and I will do the suggested update myself.

The suggested change have no functionnal impact so your
patch is OK.

Eric NOULARD <erk>
Project Administrator
Wed 17 Sep 2008 12:12:17 PM UTC, comment #6:

A typo was in my preceding comment

valueArray.erase(Rank);

should be

valueArray.erase(valueArray.begin()+Rank);

since the argument of erase should be an iterator.

Eric NOULARD <erk>
Project Administrator
Wed 17 Sep 2008 12:07:00 PM UTC, comment #5:

Hi Petr,

Fast and nice fix thanks,
I propose you some pruning.

My modification proposal would be:

1) Use meaningfull type name if possible
typedef std::string AttributeValue_t
typedef std::string ParameterValue_t
such that
vector<ParameterValue_t> will have more meaning
than
vector<std::string> in a method signature

2) Use MessageBuffer::read_string/write_string API
instead of the read_int64 + read_bytes
since it already does this (with an int32)

You should update MessageBuffer.cc since
there was a bug precisely inside the xxxx_string API :=)

3) Since we now have plain vector some of the loop (if not all)
in NetworkMessage::removeXXXX(USHort Rank)
may be replaced with STLish equivalent:
valueArray.erase(Rank);

What do you think?

Eric NOULARD <erk>
Project Administrator
Wed 17 Sep 2008 10:19:40 AM UTC, comment #4:

Fixed.

Attached patch fixes this bug and simplifies the code. (I even believe it's more efficient than before.) It works for me, so I'm happy. You can try it if you want.
I will commit (or update) this patch once my previous suggestion is accepted (or not).

(file #16509)

Petr Gotthard <gotthardp>
Project MemberIn charge of this item.
Wed 17 Sep 2008 07:53:02 AM UTC, comment #3:

When discussing bug #23150 we said that std::string is preferred wherever the new/delete char[] causes problems.

I suggest to replace char* in NetworkMessage by std::string. (If you agree, I'll make the change.)

Migrating to std::string wouldn't cause a big penalty in this case because the Values are anyway duplicated everywhere, so the performance is comparable to std::string.

Petr Gotthard <gotthardp>
Project MemberIn charge of this item.
Tue 16 Sep 2008 06:19:23 PM UTC, comment #2:

I think help is welcome,
Jean-Loup is working on it however as a "part time work".

Yes there is too many new char[xxx],
May be the API message->getValue() is a bit flawed too.
Depending on time-to-spend on this,
the fix may be small redesign (this should be an internal CERTI API) or a [may be not so] easy, avoid NULL and mem leak :=)

Eric NOULARD <erk>
Project Administrator
Tue 16 Sep 2008 04:39:27 PM UTC, comment #1:

Any update on this? Do you need a helping hand?

I used valgrind and found several strange things in the code:

(1) valgrind reports wrong memory access to a VERY_LOW address number
NetworkMessage.cc:131 and NetworkMessage.cc:138
memcpy(..., (char )(ValueArray[Rank].value), *length)

Couldn't this be caused by a (strange) dereference of ValueArray[Rank].value? It seems to me you're dereferencing char* and casting it back to (char *)

(2) after fixing this, valgrind reported wrong memory access in ObjectClassBroadcastList.cc:111
memcpy(tempValue, buffer, length)

I think buffer is NULL. I think the buffer should have been set by the message->getValue() few lines above, however when buffer is NULL, the getValue RETURNS the newly allocated buffer value. I guess this value is lost here and buffer is still NULL

(3) isn't there too many new char[length]? The buffer is allocated in message->getValue(), then it's allocated before ObjectClassBroadcastList.cc:111 again. What happens to the value allocated in message->getValue()?

Petr Gotthard <gotthardp>
Project MemberIn charge of this item.
Thu 24 Jul 2008 08:56:35 AM UTC, original submission:

Crashed on updateAttributeValues

Here are more than one federate,If federate A publishs two attributes,and federate B only subscribes
one attribute,when federate A call updateAttributeValue function, an error will be occured, and rtig will be Crashed.
my plateform is windows XP,and CERTI is compiled by vs2005.

Zheng Yanjun <zyjtsc>

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #16509:  bugfix.patch added by gotthardp (59KiB - text/x-patch - Proposed fix.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

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

     

     

    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 21 Sep 2008 10:56:15 AM UTCerkOpen/ClosedOpen=>Closed
    Fri 19 Sep 2008 01:23:41 PM UTCerkAssigned tobussenotjl=>gotthardp
      Fixed ReleaseCVS=>3.3.1
    Thu 18 Sep 2008 02:42:56 PM UTCgotthardpStatusIn Progress=>Ready For Test
    Wed 17 Sep 2008 10:19:39 AM UTCgotthardpAttached File-=>Added bugfix.patch, #16509
    Tue 16 Sep 2008 06:19:23 PM UTCerkRelease3.3.1=>3.3.0
    Tue 16 Sep 2008 04:39:27 PM UTCgotthardpSeverity4 - Important=>5 - Blocker
      Carbon-Copy-=>Added -unavailable-
    Wed 03 Sep 2008 02:29:31 PM UTCerkStatusNone=>In Progress
      Assigned toNone=>bussenotjl

    Back to the top


    Powered by Savane 3.1-cleanup1