[ROOT-7689] Crash in TGaxis::PaintAxis() in certain cases when using time format Created: 04/Oct/15  Updated: 03/Feb/16  Resolved: 03/Feb/16

Status: Closed
Project: ROOT
Component/s: Graphics
Affects Version/s: 5.34/34
Fix Version/s: 5.34/36, 6.08/00

Type: Bug Priority: Medium
Reporter: tc3t (Inactive) Assignee: Olivier Couet
Resolution: Fixed Votes: 0
Labels: None
Environment:

Source code bug, tested in Windows.


Attachments: File TGaxis.cxx    
Development:

 Description   

When using time format in axis, TGaxis::PaintAxis() may in some cases call strftime() with invalid parameter causing a crash. Below is a test case that crashes root.exe in Windows using ROOT 5.34/18:

TGraph graph;
graph.SetPoint(0, -1, 0);
graph.GetXaxis()->SetTimeFormat("%y-%m-%d %H:%M%S%F1970-01-01 00:00:00");
graph.GetXaxis()->SetTimeDisplay(1);
graph.Draw("AL");

While the input data is invalid, a crash could be easily avoided by checking return value (demonstrated in https://github.com/tc3t/qoot/commit/51190613d6eae086702ff50830099b736baf8133).



 Comments   
Comment by Olivier Couet [ 01/Feb/16 ]

I cannot reproduce it with ROOT 6

Note that the code should be:

{
   TGraph *graph = new TGraph();
   graph->SetPoint(0, -1, 0);
   graph->GetXaxis()->SetTimeFormat("%d/%m/%y %H:%M:%S%F1970-01-01 00:00:00");
   graph->GetXaxis()->SetTimeDisplay(1);
   graph->Draw("AL");
}

Comment by tc3t (Inactive) [ 01/Feb/16 ]

The underlying cause for the crash, passing nullptr to strftime, doesn't necessarily cause a crash on all compilers; demonstrations were done on VC-builds.

What was "should" referring to? A ':'-letter was missing before "%S" but couldn't spot other errors in the example code and nevertheless both examples trigger the crash.

Comment by Olivier Couet [ 02/Feb/16 ]

I applied your patch on ROOT 5.34 (attached file).
It breaks $ROOTSYS/tutorials/graphs/timeonaxis3.C

Comment by tc3t (Inactive) [ 02/Feb/16 ]

Breaks in what way? The linked code should affect only such branches that would otherwise crash on certain compilers. If breaking means fails to compile that might be because of the use of nullptr (the linked code was a commit in a fork, not a batch to ROOT 5.34-branch, so it has different language feature restrictions).

Comment by Olivier Couet [ 02/Feb/16 ]

I ran first $ROOTSYS/tutorials/graphs/timeonaxis3.C using ROOT 5.34. All the red labels were the same 2 by 2 (run the macro you will understand). They I applied your patch as in the TGaxis.cxx file I attached. Then I reran the macro with this new ROOT and the red labels were different. This macro is the test for the time axis. I won't apply any patch which makes it fail.

Comment by tc3t (Inactive) [ 02/Feb/16 ]

The linked fix code seems simple: it introduces two else branches. If neither is reached, nothing should change in the behaviour. If either is reached, it means a crash was eluded on some platform and perhaps an UB on others. With this reasoning seeing a changed behaviour suggests that the test case is broken, or the reasoning. I couldn't reach the else-branches by running the macro in compiled code (VC2012) and unsurprisingly the red labels were identical.

Comment by Olivier Couet [ 03/Feb/16 ]

Now fixed. Thanks.

Generated at Tue Sep 24 10:48:19 CEST 2019 using Jira 7.13.1#713001-sha1:5e06076c2d215a6f699b7e5c90ab2fae7ba5a1ce.