View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0000669CommunityOCCT:Application Frameworkpublic2002-09-02 19:192011-12-15 17:54
ReporteremoAssigned Tosrn 
PrioritynormalSeveritytrivial 
Status closedResolutionfixed 
OSAll 
Fixed in Version5.0.0 
Summary0000669: Standard_GUID("HoleFeature") cause stack overwrite
DescriptionThis bug has been added by fchina at
http://www.opencascade.org/forumorg/bug.php?bug_id=75&f=8 .

I have found a suttle bug in OCC, and the misleading of SampleOcaf. Since
SampleOcaf extensively use Standard_GUID("BoxDriver"), this is a big misleading
for programmers who use OCAF. I have find the logic error why using
Standard_GUID("BoxDriver") will cause crash sometimes, and why it will generate
different string when stored in file with Debug version and Release version.

Look at this function in Standard_GUID.cxx.

Standard_Integer Standard_GUID_MatchChar(const Standard_CString buffer, const
Standard_Character aChar)
{
  Standard_CString tmpbuffer = buffer;
  Standard_Integer result = -1;

  while(*tmpbuffer != '\0' && *tmpbuffer != aChar) {tmpbuffer++; result++;}

  if (result >= 0) result++;

  return result;
}

Is there any trouble with this MatchChar ? it just look for aChar in buffer.
The problem is when aChar is not in buffer, the result will beyond the scope of
buffer!

A serious programming should check the bounding condition, it should check the
ending condition, i.e. if *tmpbuffer == '\0', set result = -1. As above code, if
aChar not in buffer, result will point to the char next to '\0', it's dangerous!

Look these two functions: (it is called in constructor)

Standard_CString Standard_GUID_GetValue32(const Standard_CString tmpBuffer,
Standard_Integer& my32b)
{
  Standard_Character strtmp[Standard_GUID_SIZE_ALLOC];
  Standard_Integer pos = 0;

  pos = Standard_GUID_MatchChar(tmpBuffer,'-');
  if (pos >= 0) {
    strncpy(strtmp,tmpBuffer,pos);
    strtmp[pos] = '\0';
    my32b = (Standard_Integer) strtoul(strtmp, (char **)NULL, 16);
  }

  return &tmpBuffer[pos+1];
}

Standard_CString Standard_GUID_GetValue16(const Standard_CString tmpBuffer,
Standard_Integer& my32b)
{
  Standard_Character strtmp[Standard_GUID_SIZE_ALLOC];
  Standard_Integer pos = 0;

  pos = Standard_GUID_MatchChar(tmpBuffer,'-');
  if (pos >= 0) {
    strncpy(strtmp,tmpBuffer,pos);
    strtmp[pos] = '\0';
    my32b = (Standard_Integer) strtoul(strtmp, (char **)NULL, 16);
  }

  return &tmpBuffer[pos+1];
}

No problem, isn't it?

Then assume when construct a Standard_GUID("HoleFeature") in stack. (this is a
common case just as SampleOcaf) by the constructor:

Standard_GUID::Standard_GUID(const Standard_CString aGuid)
: my32b ( 0),
  my16b1 ( 0),
  my16b2 ( 0),
  my16b3 ( 0),
  my8b1 ( 0),
  my8b2 ( 0),
  my8b3 ( 0),
  my8b4 ( 0),
  my8b5 ( 0),
  my8b6 ( 0)
{
  Standard_CString tmpBuffer = aGuid;
   
  tmpBuffer = Standard_GUID_GetValue32(tmpBuffer,my32b);
  tmpBuffer = Standard_GUID_GetValue16(tmpBuffer,my16b1);
  tmpBuffer = Standard_GUID_GetValue16(tmpBuffer,my16b2);
  tmpBuffer = Standard_GUID_GetValue16(tmpBuffer,my16b3);
  tmpBuffer = Standard_GUID_GetValue8(tmpBuffer,my8b1);
  tmpBuffer = Standard_GUID_GetValue8(tmpBuffer,my8b2);
  tmpBuffer = Standard_GUID_GetValue8(tmpBuffer,my8b3);
  tmpBuffer = Standard_GUID_GetValue8(tmpBuffer,my8b4);
  tmpBuffer = Standard_GUID_GetValue8(tmpBuffer,my8b5);
  tmpBuffer = Standard_GUID_GetValue8(tmpBuffer,my8b6);
}

The parameter passed into Standard_GUID is "HoleFeature", no '-', so tmpBuffer
has already pointed to incorrect content! And unfortunely, all those const
string in stack is allocated in some static segment by compiler, and this time
following "HoleFeature" is a long string, its size greater than
Standard_GUID_SIZE_ALLOC,
so when execute this code:

    strncpy(strtmp,tmpBuffer,pos);

    auto variable pos is overwrtten by strncpy functin! Debugger display pos as
a big integer( in hex, just a part of that long string and begin with length of
Standard_GUID_SIZE_ALLOC, so prove a buffer overwritting.), then

   strtmp[pos] = '\0';

   cause OS throw access violation.


The solution for OCC developer should of course check the loop end condition to
see *tmpBuffer = '\0'. if it is, return -1.
And in construtor should add a check: CheckGUIDFormat, if return false, throw
exception. (at least #define in debug version)

For application developer, change all code using Standard_GUID that follows
SampleOCAF, and always use tools to generate GUID and copy them into code. .

fhchina
TagsNo tags attached.
Test case number

Attached Files

  • 10_SampleOcaf.zip (239,248 bytes)
  • Standard_GUID.zip (2,073 bytes)
  • OCC669-OCC738-OCC739.zip (9,471 bytes)

Relationships

related to 0000738 closedptv Open CASCADE The GUIDs in XCAFDoc have incorrect format. 
related to 0000739 closedsrn Open CASCADE Invalid GUIDs in DDataStd_DrawPresentation and DDataStd_Sample. 

Activities

2002-09-05 07:39

 

10_SampleOcaf.zip (239,248 bytes)

2002-09-11 14:32

 

Standard_GUID.zip (2,073 bytes)

2002-09-17 15:22

 

OCC669-OCC738-OCC739.zip (9,471 bytes)

Issue History

Date Modified Username Field Change
2002-09-03 12:43 bugmaster Assigned To bugmaster => szy
2002-09-03 12:43 bugmaster Status new => assigned
2002-09-03 12:43 bugmaster Summary => Standard_GUID("HoleFeature") cause stack overwrite
2002-09-03 18:12 szy Assigned To szy => srn
2002-09-05 11:42 srn Status assigned => resolved
2002-09-11 18:45 bugmaster CC => apv
2002-09-11 19:47 apv CC => aki
2002-09-17 13:14 vtn OtherBugsDependingOnThis => 738, 739
2002-09-23 18:37 aki Status resolved => tested
2002-10-01 17:43 bugmaster Status tested => closed
2002-10-01 17:43 bugmaster Resolution @0@ => fixed
2011-08-02 10:32 bugmaster Category OCCT:OCAF => OCCT:Application Framework
2011-12-15 17:54 bugmaster Project Open CASCADE => Community