Add a New Comment (Rich Markup)
( Jump to the original submission )
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").
IIUC, those functions aren't in the standard until C++23: https://en.cppreference.com/w/c/chrono
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.
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.
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.
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.
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: See comment 23 at bug #53539 for an answer
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.
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.
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.
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.
(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)
Attach Files: Comment:
Depends on the following items: None found
Items that depend on this one: None found
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 project members can vote.
Please enter the title of George Orwell's famous dystopian book (it's a date):
Follow 3 latest changes.
Copyright © 2023 Free Software Foundation, Inc. Verbatim copying and distribution of this entire article is permitted in any medium, provided this notice is preserved. The Levitating, Meditating, Flute-playing Gnu logo is a GNU GPL'ed image provided by the Nevrax Design Team. Source Code
Powered by Savane 3.12