bugGNU Octave - Bugs: bug #55437, Files in editor are reported as...

 
 

bug #55437: Files in editor are reported as modified while heavily parsing date strings with datenum()

Submitter:  Lars Kindermann <larskindermann>
Submitted:  Thu 10 Jan 2019 11:06:39 AM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Need Info Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 20 May 2021 04:19:28 PM UTC, comment #11: 

Looking at the code a little bit more, I don't think that our usage of ctime, std::localtime, and std::gmtime is causing the problem here.
I also think that we are using those functions safely. (But not 100% certain.)

BTW: I should have written "those functions aren't in the standard until C23" (not "C++23").

Markus Mützel <mmuetzel>
Group administrator
Thu 20 May 2021 03:25:38 PM UTC, comment #10: 

IIUC, those functions aren't in the standard until C++23:
https://en.cppreference.com/w/c/chrono

Markus Mützel <mmuetzel>
Group administrator
Thu 20 May 2021 03:19:44 PM UTC, comment #9: 

Unless there are standard C++ functions to handle this job, then we should probably be using the gnulib time_r module and some wrapper functions.

John W. Eaton <jwe>
Group administrator
Thu 20 May 2021 03:10:06 PM UTC, comment #8: 

GitHub CodeQL generated the following reports that might be related to this issue:
https://github.com/gnu-octave/octave/security/code-scanning

Use of potentially dangerous function
liboctave/system/oct-time.cc#L278 • Detected 17 days ago by CodeQL

Use of potentially dangerous function
liboctave/system/oct-time.cc#L288 • Detected 17 days ago by CodeQL

Use of potentially dangerous function
libinterp/corefcn/__ftp__.cc#L176 • Detected 17 days ago by CodeQL


The reports contain the following details:

> This rule finds calls to functions that are dangerous to use. Currently, it checks for calls to gmtime, localtime, ctime and asctime.
>
> The time related functions such as gmtime fill data into a tm struct or char array in shared memory and then returns a pointer to that memory. If the function is called from multiple places in the same program, and especially if it is called from multiple threads in the same program, then the calls will overwrite each other's data.
>
> = Recommendation =
> Replace calls to gmtime with gmtime_r. With gmtime_r, the application code manages allocation of the tm struct. That way, separate calls to the function can use their own storage.
>
> Similarly replace calls to localtime with localtime_r, calls to ctime with ctime_r and calls to asctime with asctime_r.
>
> = Example =
> The following example checks the local time in two ways:


// BAD: using gmtime
int is_morning_bad() {
    const time_t now_seconds = time(NULL);
    struct tm *now = gmtime(&now_seconds);
    return (now->tm_hour < 12);
}

// GOOD: using gmtime_r
int is_morning_good() {
    const time_t now_seconds = time(NULL);
    struct tm now;
    gmtime_r(&now_seconds, &now);
    return (now.tm_hour < 12);
}


> The first version uses gmtime, so it is vulnerable to its data being overwritten by another thread. Even if this code is not used in a multi-threaded context right now, future changes may make the program multi-threaded. The second version of the code uses gmtime_r. Since it allocates a new tm struct on every call, it is immune to other calls to gmtime or gmtime_r.


Markus Mützel <mmuetzel>
Group administrator
Thu 24 Jan 2019 07:39:03 PM UTC, comment #7: 

Ah, there's a test failure in "test datevec". My patch breaks the millisecond-handling hack.

I'll look in to it. Sorry for not testing first.

Andrew Janke <apjanke>
Thu 24 Jan 2019 07:29:47 PM UTC, comment #6: 

Andrews patch fixed the popup messages and timstamp changes in the file browser, as expected when removing the TZ changes.

But I am not sure wether just handing GMT0 over to strptime really solves all conversion problems. Did you try all the tests and assertions in datevec.m?

An what happens if the PC clock itself is in the daylight saving period, when the clock is just turned back? We have to make sure everything is always correct.

Lars Kindermann <larskindermann>
Mon 21 Jan 2019 12:41:02 AM UTC, comment #5: 

Here's a patch to try out the "munge with explicit time zone" approach: file #46052:  datevec-munge-strings-instead-of-TZ-env-var.patch. Lars, want to try this out?

Negligible effect on parsing speed. Before:


>> bench_datenum
Parsed 5000 dates in 0.972425938 s, 194 usec per date
>> bench_datenum (50000)
Parsed 50000 dates in 9.472014904 s, 189 usec per date


After:


>> bench_datenum
Parsed 5000 dates in 0.983253002 s, 197 usec per date
>> bench_datenum (50000)
Parsed 50000 dates in 9.850703001 s, 197 usec per date


There was one test it broke when I tried it out. But that was for `eigs`, which doesn't even use dates as far as I can tell. I suspect that was just  an existing intermittent failure cropping up.

Andrew Janke <apjanke>
Sat 12 Jan 2019 11:48:17 AM UTC, comment #4: 

@Andrew: See comment 23 at bug #53539 for an answer

Lars Kindermann <larskindermann>
Sat 12 Jan 2019 04:46:47 AM UTC, comment #3: 

Makes sense that the temporary TZ change would cause this. I can't reproduce on macOS, but I think that's because macOS does its file change notifications differently, subscribing to a system service that pushes filesystem change notification events.

Maybe there's a way to do the "zone-independent" date parsing locally instead of fiddling with the global TZ environment variable: Check the date format string, and if it doesn't include an explicit `%z` or `%Z`, then concatenate ' %Z' to the format string and ' GMT' to the input date strings, so the strptime() parsing logic doesn't have to refer to TZ at all.

Andrew Janke <apjanke>
Fri 11 Jan 2019 03:52:10 PM UTC, comment #2: 

Ubuntu 16.04, Core i5, 4Gig RAM, Ext4 on HD
Just 5 open files in the current folder
While the script is running, I get up to three warnings popping up

Additionally, I see the dates in the file-browser erratically changing while the script is running.

In bug #53539 this was caused by changing the timezone temporarily within datenum() and the gui asynchronically checking filedates at the same time.

Lars Kindermann <larskindermann>
Fri 11 Jan 2019 02:56:05 PM UTC, comment #1: 

I modified your script to generate N open m-files easily.


N = 100;
list = dir ('/path/to/repo/scripts/*/*.m');
list = strcat({list.folder}', filesep (), {list.name}');
cellfun (@edit, list(1:N))

disp preparing
for i=1:100000
 s(i,:)='20010101-000000';
end
disp starting
t=datenum(s,'yyyymmdd-HHMMSS');
disp done


But I cannot reproduce the problem on openSUSE 15.0, Intel Core i5, 16 GB RAM.


>> __octave_config_info__ ("hg_id")
ans = 02a9e1bb314a


You speak of slow systems, what does this mean for you?  Do you store the files on some special location?  The opened files are stored on a XFS partition.


Kai Torben Ohlhus <siko1056>
Group Member
Thu 10 Jan 2019 11:06:39 AM UTC, original submission:  

Reappearing behaviour of bug #53539, which was fixed a year ago.

How to trigger: Open some files in the editor (the more the better), including this script


disp preparing
for i=1:100000
 s(i,:)='20010101-000000';
end
disp starting
t=datenum(s,'yyyymmdd-HHMMSS');
disp done


Running this script, during datenum() the GUI sometimes pops up a message box, stating that some file opened in editor has changed.

It happens more frequently on a slower CPU.

Lars Kindermann <larskindermann>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by apjanke (Posted a comment)
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by larskindermann (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-02-26 mtmiller Release5.0.1 dev
    2019-01-21 apjanke Attached File- Added datevec-munge-strings-instead-of-TZ-env-var.patch, #46052
    2019-01-11 siko1056 StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code