bugCERTI - Bugs: bug #23921, Crashed on updateAttributeValues

 
 

bug #23921: Crashed on updateAttributeValues

Submitter:  Zheng Yanjun <zyjtsc>
Submitted:  Thu 24 Jul 2008 08:56:35 AM UTC
   
 
Category:  CERTI Severity:  * 5 - Blocker
Item Group:  Software error Status:  Ready For Test
Privacy:  Public Assigned to:  gotthardp
Open/Closed:  Closed Release:  * 3.3.0
Reproducibility:  Every Time Fixed Release:  * 3.3.1
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group administrator
Thu 18 Sep 2008 02:42:56 PM UTC, comment #10: 

Fixed.

Petr Gotthard <gotthardp>
Group Member
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>
Group 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>
Group Member
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>
Group 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>
Group 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>
Group 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>
Group Member
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>
Group Member
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>
Group 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>
Group Member
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 Files:
   
   
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
  • -email is unavailable- added by gotthardp (Posted a comment)
  • -email is unavailable- added by gotthardp
  • -email is unavailable- added by erk (Updated the item)
  • -email is unavailable- added by zyjtsc (Submitted the item)
  •  

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-09-21 erk Open/ClosedOpen Closed
    2008-09-19 erk Assigned tobussenotjl gotthardp
        Fixed ReleaseCVS 3.3.1
    2008-09-18 gotthardp StatusIn Progress Ready For Test
    2008-09-17 gotthardp Attached File- Added bugfix.patch, #16509
    2008-09-16 erk Release3.3.1 3.3.0
    2008-09-16 gotthardp Severity4 - Important 5 - Blocker
        Carbon-Copy- Added -email is unavailable-
    2008-09-03 erk StatusNone In Progress
        Assigned toNone bussenotjl

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code