Test perf bop boxholes crashes DRAW

Problem described in issue is reproduced on current state of OCCT.

There are two different problems actually:

1. OCCT problem

Method OSD_File::Capture() calls MSVCRT function _open_osfhandle() to get C file descriptor to the file opened using WinAPI function. This descriptor is created by that function and then never closed. This leads to overflow of the table of descriptors and the application gets terminated by C library (silent exit after definite number of DRAW commands captured by dlog).

2. Tcl problem

On Windows, standard channels (stdin, stdout, stderr) in Tcl are initialized by OS handles returned by WinAPI function GetStdHandle() (see TclpGetDefaultStdChannel() defined in win/tclWinChan.c).

These handles may be invalidated (closed, reopened, reassigned to different kind of object) by C/C++ code. In particular, function _dup2() of standard C library (MSVC), when called for the standard file number (0, 1, 2) as second argument, closes the OS handle associated with the standard stream, then creates the new handle and sets is as standard one by call to SetStdHandle().

In most cases the old and new handles are the same (apparently due to reuse), thus there are no immediate consequences. However, sometimes (in my experiments about once per several thousand calls), the new standard handle assigned by the system is different from the old one. Yet Tcl channel still keeps the old handle and when trying to use that channel (e.g. use puts to write to stdout), error occurs.

In this context, sometimes execution of a test script ends up with Tcl reporting
"error writing "stdout": bad file number"

For this problem, a ticket is created on Tcl:

https://core.tcl.tk/tcl/tktview/[revision removed]

The proposed solution is to duplicate the standard handle returned by GetStdHandle() (in TclpGetDefaultStdChannel()) and to use the duplicate for initialization of the Tcl channel.

win/tclWinChan.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/win/tclWinChan.c b/win/tclWinChan.c
index 8c47be646..480b387f4 100644
--- a/win/tclWinChan.c
+++ b/win/tclWinChan.c
@@ -1302,6 +1302,17 @@ TclpGetDefaultStdChannel(
     return (Tcl_Channel) NULL;
     }

+ /*
+ * Make duplicate of the standard handle as it may be altered
+ * (closed, reopened with another type of the object etc.) by
+ * the system or a user code at any time, e.g. by call to _dup2()
+ */
+ if (! DuplicateHandle (GetCurrentProcess(), handle,
+ GetCurrentProcess(), &handle,
+ 0, FALSE, DUPLICATE_SAME_ACCESS)) {
+ return (Tcl_Channel) NULL;
+ }
+
     channel = Tcl_MakeFileChannel(handle, mode);

    if (channel == NULL) {

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Sat Nov 17 12:51:26 2018 +0300

    0027620: Test perf bop boxholes crashes DRAW
    
    1. Method OSD_File::Capture() is removed: on Windows it was allocating a C file descriptor for a file opened using WinAPI, and never released that descriptor (once allocated, it cannot be released separately from WinAPI file handle). Direct calls to dup/dup2 are used instead.
    
    2. In Draw_Window.cxx the standard Tcl channels are initialized manually using corrected version of Tcl internal function. This works around a problem with Tcl channels on Windows being bound to OS device handle owned by the system which can get invalidated as result of calls to dup2() (used to capture output to standard streams).
    
    3. Implementation of capturing of output to standard streams in DRAW (see command dlog) is revised:
    - Temporary file for capturing is opened once and used to store whole log, thus the need to collect log in the string stream in memory is avoided
    - Errors at capturing cout are handled and reported
    
    Test demo draw dlog is added
    Test demo draw getsource is corrected for VS2017 which generates file name in lowercase

Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Sat Nov 17 21:43:51 2018 +0300

    Fixes for Linux

Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Sat Nov 17 22:54:34 2018 +0300

    fix test

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

Branch [archived branch] has been updated forcibly by Author.

[revision removed]

The fix (resolving #0027620, #0027043, #0027013) is pushed to branch CR27620 and tested, see Jenkins job CR27620-master-abv. Please review

src/Draw/Draw_Interpretor.hxx
- rename "GetLogFileDescripror" to "GetLogFileDescriptor".

Why this difference presents on Linux?

IMAGE bugs modalg_5 bug24012: bug24012.png differs

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Sat Nov 17 12:51:26 2018 +0300

    0027620: Test perf bop boxholes crashes DRAW
    
    Implementation of capturing of output to standard streams in DRAW (see command dlog) is revised to avoid problems with command "test" executing long test scripts:
    
    1. Method OSD_File::Capture() is removed: on Windows it was allocating a C file descriptor for a file opened using WinAPI, and never released that descriptor (once allocated, it cannot be released separately from WinAPI file handle). Direct calls to dup/dup2 are used instead.
    
    2. In Draw_Window.cxx the standard Tcl channels are initialized manually using corrected version of Tcl internal function. This works around a problem with Tcl channels on Windows being bound to OS device handle owned by the system which can get invalidated as result of calls to dup2() (used to capture output to standard streams).
    
    3. Temporary file for capturing is opened once and used to store whole log, thus the need to collect log in the string stream in memory is avoided
    
    4. Possible errors of dup() and dup2() are checked and reported
    
    Test demo draw dlog is added
    Test demo draw getsource is corrected for VS2017 which generates file name in lowercase

Branch CR27620_1 contains renaming of GetLogFileDescripror. The tests are re-executed in the same Jenkins job. Alas, image for bug24012 is still empty on Debian. I have no idea why (the fix should not affect that test in any way), to be checked.

I have tried to change the script bug24012:

 vinit
-OCC24012 face edge
+nproject r edge face
 vsetdispmode 0
 vdisplay face
 vdisplay edge
+vdisplay r
+vsetcolor r yellow
 vfit

This solves the problem.
The changed script still reproduces the bug #0024012 in OCCT 6.9.1. So, I think the synthetic command OCC24012 is not needed at all.

In the command OCC24012, the result shape is displayed in the AIS viewer. If we suppress displaying the shape the problem is gone:

         Handle(AIS_InteractiveObject) myShape = new AIS_Shape (rshape);
         myAISContext->SetColor (myShape, Quantity_Color(Quantity_NOC_YELLOW), Standard_False);
-		myAISContext->Display (myShape, Standard_True);
+		//myAISContext->Display (myShape, Standard_True);

It is strange that this patch impacts displaying a shape from within a draw command.
The jenkins result is http://jenkins-test-12.nnov.opencascade.com/view/CR0-27620-master-MSV/view/TESTING/job/CR0-27620-master-MSV-OCCT-Debian80-64-opt-test-restart/HTML_20Report/

Thanks a lot Mikhail for your investigation! Yet have you been able to reproduce the problem on Linux (i.e. run test and see empty viewer)? On my side I have tried this on Ubuntu 16.04 with CLang and it worked without problem.

Test bugs modalg_5 bug24012 does not seem to check anything, IMHO it should be reworked to not only use nproject instead of specific DRAW command, but also to check the resulting shape. Do you agree with that?

I agree with you to revise the test bug24012.

I tested on Linux Debian 8 on current Jenkins-test-12 both my above changes. Both of them solve the problem with empty screen. And after reverting changes empty screen appeared again.

I think before closing this bug we should understand why this patch impacts behavior of the test when a shape is displayed from within the draw command OCC24012.

Valgrind report:

==9803== Conditional jump or move depends on uninitialised value(s)
==9803== at 0x1180AA45: BRepAlgo_NormalProjection::Build() (BRepAlgo_NormalProjection.cxx:474)
==9803== by 0x1A0FAC50: OCC24012(Draw_Interpretor&, int, char const**) (QABugs_19.cxx:1291)
...

The problem is in uninitialized field myFaceBounds. When it happens to be zero, the edge with bad BSpline curve is created (internal poles have oscillating coordinates with values as high as ~ 1e14). When that curve is displayed in 3d viewer, it shows nothing after fit all.

To reproduce this in stable way, add option "-g" to nproject command:

nproject r edge face -g

Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Fri Nov 23 00:43:37 2018 +0300

    Field myFaceBounds is initialized in constructor of the class BRepAlgo_NormalProjection to avoid undefined behavior; test bugs modalg_5 bug24012 is corrected to use command nproject instead of custom one, and to check propertes of the resulting shape

Branch [archived branch] has been updated by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Fri Nov 23 01:08:32 2018 +0300

    Fix gcc warnings

Branch [archived branch] has been created by Author.

[revision removed]


Detailed log of new commits:

Author: Author
Date: Sat Nov 17 12:51:26 2018 +0300

    0027620: Test perf bop boxholes crashes DRAW
    
    Implementation of capturing of output to standard streams in DRAW (see command dlog) is revised to avoid problems with command "test" executing long test scripts:
    
    1. Method OSD_File::Capture() is removed: on Windows it was allocating a C file descriptor for a file opened using WinAPI, and never released that descriptor (once allocated, it cannot be released separately from WinAPI file handle). Direct calls to dup/dup2 are used instead.
    
    2. In Draw_Window.cxx the standard Tcl channels are initialized manually using corrected version of Tcl internal function. This works around a problem with Tcl channels on Windows being bound to OS device handle owned by the system which can get invalidated as result of calls to dup2() (used to capture output to standard streams).
    
    3. Temporary file for capturing is opened once and used to store whole log, thus the need to collect log in the string stream in memory is avoided
    
    4. Possible errors of dup() and dup2() are checked and reported
    
    Test demo draw dlog is added
    
    Off-topic changes:
    
    - Test demo draw getsource is corrected for VS2017 which generates file name in lowercase
    - Field myFaceBounds is initialized in constructor of the class BRepAlgo_NormalProjection to avoid undefined behavior
    - Test bugs modalg_5 bug24012 is corrected to use command nproject instead of custom one, and to check propertes of the resulting shape

Please review corrected branch CR27620, the same Jenkins job.

Regarding #0024012, please decide on whether it should be reopened. In my understanding, the problem reported in that issue (bad curve) has never been solved. Yet the reproducer was depending on uninitialized field, and over time the code drifted to the state that it did not trigger wrong behavior in the tests. Still the originally reported erroneous behavior can be reproduced by adding -g to nproject command in the test script.

I think the issue #0024012 should not be reopened. The option SetLimit() should be used in such cases. If the caller is confident that the projection fits in surface boundaries he can save time by disabling boundaries checking.

Reviewed.

Combination -
OCCT branch : [archived branch] SHA - [revision removed]
Products branch : [archived branch] SHA - [revision removed]
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 16316.060000000056 / 16331.970000000008 [-0.10%]
Products
Total CPU difference: 7041.690000000034 / 7059.160000000035 [-0.25%]
Windows-64-VC14:
OCCT
Total CPU difference: 17779.1875 / 17739.59375 [+0.22%]
Products
Total CPU difference: 8513.953125 / 8542.921875 [-0.34%]

Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]

Branch [archived branch] has been deleted by Participant.

[revision removed]