cryptobiu / MATRIX

MPC Simulation Framework
MIT License
24 stars 13 forks source link

Issues in Matrix Logger Class #27

Closed lenerd closed 5 years ago

lenerd commented 5 years ago

Hi, there are multiple issues with the provided logger class:


https://github.com/cryptobiu/MATRIX/blob/6fe73ad9e0b2890c8d8c0cecabe1aace79bb6f3b/Reporting/MatrixMeasurement.h#L20-L24 The variable buff is used uninitialized. Depending on the concrete value, getcwd might overwrite some other data or the program might crash with a segmentation fault. Something like the following code should be used instead.

string getcwdStr() 
{ 
    char buff[256];
    auto res = getcwd(buff, 256);
    assert(res != NULL);
    return string(res);
} 

https://github.com/cryptobiu/MATRIX/blob/6fe73ad9e0b2890c8d8c0cecabe1aace79bb6f3b/Reporting/MatrixMeasurement.h#L84-L96

When computing the time for a task in line 91, the code subtracts always the starting time of the last task from the ending time of the current task. This can result in wrong and sometimes negative values for the task durations. The line should be changed to:

logFile << to_string(m_cpuEndTimes[idx][idx2]- m_cpuStartTimes[idx][idx2])

You can find these changes among some others here: https://github.com/lenerd/ABY/blob/matrix/src/abycore/MATRIX/MatrixMeasurement.h

Also:

liorko87 commented 5 years ago

Hi,

I made some changes to the logger class, based on your code snippets.

As for your last question

Also:

  • What is the reason to use an array of C strings for the arguments?

The reason to use array of C strings is to keep existing API in our C++ core library.

lenerd commented 5 years ago

Ok, then I guess it does not hurt anyone if I change the API in the corresponding ABY header, right?

liorko87 commented 5 years ago

No problem at all.