Bug 280922 - Change 46c599340f187db577b9212ab18022f3c7380c68 fixes one case (localtime / gmtime -> strftime('%s'), but breaks another
Summary: Change 46c599340f187db577b9212ab18022f3c7380c68 fixes one case (localtime / ...
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 14.1-STABLE
Hardware: Any Any
: --- Affects Many People
Assignee: freebsd-bugs (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-08-19 11:28 UTC by Vladislav Shabanov
Modified: 2024-08-19 18:18 UTC (History)
3 users (show)

See Also:


Attachments
example code (554 bytes, text/plain)
2024-08-19 11:28 UTC, Vladislav Shabanov
no flags Details
the patch (465 bytes, patch)
2024-08-19 11:29 UTC, Vladislav Shabanov
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vladislav Shabanov 2024-08-19 11:28:51 UTC
Created attachment 252911 [details]
example code

Change 46c599340f187db577b9212ab18022f3c7380c68 fixes one case (localtime / gmtime -> strftime('%s'), but breaks another:

Classic POSIX specification says nothing about tm_zone and tm_gmtoff. A lot of code assumes, than only described in the specification fields of struct tm affect to the result of strftime. 

For example, Python has timetuple described in package "time", containing these fields. The datetime module constructs timetuple to convert the datetime object to string (see https://github.com/python/cpython/blob/v3.12.5/Modules/_datetimemodule.c date_strftime then datetime_timetuple, then https://github.com/python/cpython/blob/v3.12.5/Modules/timemodule.c )

Change https://cgit.freebsd.org/src/commit/lib/libc/stdtime?id=46c599340f187db577b9212ab18022f3c7380c68 fixes case discovered by  Dag-Erling Smørgrav but changes the behaviour when struct tm is constructed manually in assumption, than the structure contains only standard fields.

IMHO, there is another solution: both localtime and gmtime fill not only tm->tm_gmtoff, but also tm->tm_zone. Hence, it's possible to distinguish two cases: 
struct tm filled by library code and struct tm filled manually. 

Yes, I know that strftime('%s') is undocumented feature in Python, but I sure that this is only one case. I sure that a lot of code assumes than struct tm is based in local timezone, not UTC.

The example code ant proposed patch solving the issue are attached.
Comment 1 Vladislav Shabanov 2024-08-19 11:29:43 UTC
Created attachment 252912 [details]
the patch
Comment 2 Dag-Erling Smørgrav freebsd_committer freebsd_triage 2024-08-19 13:58:40 UTC
Your patch makes no sense.  You assume that fields that weren't explicitly initialized are zero, but that's frequently not the case.  The correct fix is to change cpython to explicitly initialize its `struct tm` either with an initializer or with `memset()`.
Comment 3 Vladislav Shabanov 2024-08-19 14:35:45 UTC
(In reply to Dag-Erling Smørgrav from comment #2)
Thank you for reply.

First of all, the current implementation of libc will produce completely wrong result when someone initializes only standard fields:
  struct tm x;
  x.tm_hour = 1;
  x.tm_min = 2;
  x.tm_sec = 3;
  x.tm_mday = 4;
  x.tm_mon = 6;
  x.tm_year = 106;
  x.tm_isdst = -1;
  strftime(....)


This is because tm_gmtoff can get any value. In my first test, I got ~10 years shift just because follow the C reference.

That's why most portable code does some equivalent of bzero(&x), including Python. Python, like many other big codebases, is tolerant to added, but unused fields.
Certainly, Python does bzero.

Secondly, there is no way change Python to adopt it to new behaviour of strftime: the time and timetouple API is standard, nobody will add extra fields to it, because it will break a lot of Python code. Furthermore, it's impossible to fix the problem at Python level (restore  tm_gmtoff), because it lost earlier, when datetime is converted to timetouple. 

To sum up, now we have implementation with two problems:
1) it gives absolutely wrong result with badly written program initializing only  documented fields (I mean, in POSIX standard) and leaves tm_gmtoff initialized. I sure that there are a lot of code written in last 50 years doing so.
2) It changes assumption that the unpacked struct tm is treated as local time, which is mentioned in manual, and gives unexpected result when someone construct struct tm manually without the knowledge of tm_gmtoff.

The proposed patch solves only the second problem. Maybe, it makes sence to change
(t->tm_gmtoff || t->tm_zone)  to (t->tm_gmtoff && t->tm_zone) to fix both cases.

Anyway, this patch keeps your original problem solved, because gmtime and localtime fills both tm_gmtoff and tm_zone and new behaviour breaks less existing code.
Comment 4 Vladislav Shabanov 2024-08-19 14:54:19 UTC
(In reply to Vladislav Shabanov from comment #3)
one my mistake to fix in previous comment: 

both the current implementation and the patched one keeps strftime producing completely wrong result when somebody initializes only standard fields. But current code breaks all programs which carefully clear struct by bzero(), but the patch proposed does not.