Add timezone offset to new document creation date [pdfium : main]
Kuka (Gerrit)
Kuka has uploaded the change for review![]( "Open in Gerrit")
Commit message
Add timezone offset to new document creation date
FPDF_CreateNewDocument() writes CreationDate using the local time, but omitted the UTC offset. Include the offset in the PDF date string when machine time is available.
Add a deterministic regression test for a non-hour timezone offset and update the saved document expectation for the longer date string.
Change diff
diff --git a/fpdfsdk/fpdf_edit_embeddertest.cpp b/fpdfsdk/fpdf_edit_embeddertest.cpp
index 6e66cf7..8787bf2 100644
--- a/fpdfsdk/fpdf_edit_embeddertest.cpp
+++ b/fpdfsdk/fpdf_edit_embeddertest.cpp
@@ -4,6 +4,7 @@
#include <stdint.h>
+#include <time.h>
#include <array>
#include <limits>
#include <memory>
@@ -32,7 +33,9 @@
#include "fpdfsdk/cpdfsdk_helpers.h"
#include "public/cpp/fpdf_scopers.h"
#include "public/fpdf_annot.h"
+#include "public/fpdf_doc.h"
#include "public/fpdf_edit.h"
+#include "public/fpdf_ext.h"
#include "public/fpdfview.h"
#include "testing/embedder_test.h"
#include "testing/embedder_test_constants.h"
@@ -71,6 +74,24 @@
constexpr char kNotoSansScPng[] = "noto_sans_sc";
+time_t FakeTime() {
+ return 1587611121; // 2020-04-23 03:05:21 UTC.
+}
+
+tm* FakeLocaltime(const time_t* time) {
+ static tm fake_local_time = {};
+ if (*time != FakeTime()) {
+ return nullptr;
+ }
+ fake_local_time.tm_year = 2020 - 1900;
+ fake_local_time.tm_mon = 4 - 1;
+ fake_local_time.tm_mday = 23;
+ fake_local_time.tm_hour = 10;
+ fake_local_time.tm_min = 35;
+ fake_local_time.tm_sec = 21;
+ return &fake_local_time;
+}
+
struct FPDFEditMoveEmbedderTestCase {
std::vector<int> page_indices;
int page_indices_len;
@@ -269,14 +290,14 @@
"0000000017 00000 n\r\n"
"0000000066 00000 n\r\n"
"0000000122 00000 n\r\n"
- "0000000192 00000 n\r\n"
+ "0000000199 00000 n\r\n"
"trailer\r\n"
"<<\r\n"
"/Root 1 0 R\r\n"
"/Info 3 0 R\r\n"
"/Size 5/ID\\[<.*><.*>\\]>>\r\n"
"startxref\r\n"
- "285\r\n"
+ "292\r\n"
"%%EOF\r\n";
} // namespace
@@ -379,6 +400,19 @@
kExpectedPDF, sizeof(kExpectedPDF))));
}
+TEST_F(FPDFEditEmbedderTest, NewDocumentCreationDateHasTimeZone) {
+ FSDK_SetTimeFunction(FakeTime);
+ FSDK_SetLocaltimeFunction(FakeLocaltime);
+ ScopedFPDFDocument doc(FPDF_CreateNewDocument());
+ FSDK_SetTimeFunction(nullptr);
+ FSDK_SetLocaltimeFunction(nullptr);
+ ASSERT_TRUE(doc);
+
+ unsigned short buf[128];
+ ASSERT_EQ(48u, FPDF_GetMetaText(doc.get(), "CreationDate", buf, sizeof(buf)));
+ EXPECT_EQ(L"D:20200423103521+07'30'", GetPlatformWString(buf));
+}
+
// Regression test for https://crbug.com/667012
TEST_F(FPDFEditEmbedderTest, RasterizePDF) {
const char kAllBlackPng[] = "black_612x792";
diff --git a/fpdfsdk/fpdf_editpage.cpp b/fpdfsdk/fpdf_editpage.cpp
index 7b5613e..c1b7905 100644
--- a/fpdfsdk/fpdf_editpage.cpp
+++ b/fpdfsdk/fpdf_editpage.cpp
@@ -51,6 +51,10 @@
namespace {
+constexpr int kMinutesPerHour = 60;
+constexpr int kHoursPerDay = 24;
+constexpr int kMinutesPerDay = kMinutesPerHour * kHoursPerDay;
+
static_assert(FPDF_PAGEOBJ_TEXT ==
static_cast<int>(CPDF_PageObject::Type::kText),
"FPDF_PAGEOBJ_TEXT/CPDF_PageObject::TEXT mismatch");
@@ -171,6 +175,51 @@
return {params, page_obj};
}
+int64_t DaysSinceEpoch(const tm& time) {
+ int year = time.tm_year + 1900;
+ const int month = time.tm_mon + 1;
+ const int day = time.tm_mday;
+ year -= month <= 2 ? 1 : 0;
+ const int era = (year >= 0 ? year : year - 399) / 400;
+ const unsigned year_of_era = static_cast<unsigned>(year - era * 400);
+ const unsigned month_of_year =
+ static_cast<unsigned>(month > 2 ? month - 3 : month + 9);
+ const unsigned day_of_year =
+ (153 * month_of_year + 2) / 5 + static_cast<unsigned>(day) - 1;
+ const unsigned day_of_era =
+ year_of_era * 365 + year_of_era / 4 - year_of_era / 100 + day_of_year;
+ return era * 146097 + static_cast<int>(day_of_era) - 719468;
+}
+
+int TimeZoneOffsetInMinutes(const tm& local_time, const tm& utc_time) {
+ const int64_t day_offset =
+ DaysSinceEpoch(local_time) - DaysSinceEpoch(utc_time);
+ return static_cast<int>(day_offset * kMinutesPerDay +
+ (local_time.tm_hour - utc_time.tm_hour) *
+ kMinutesPerHour +
+ local_time.tm_min - utc_time.tm_min);
+}
+
+ByteString FormatPDFDate(time_t current_time, tm local_time) {
+ tm* utc_time = gmtime(¤t_time);
+ if (!utc_time) {
+ return ByteString::Format("D:%04d%02d%02d%02d%02d%02d",
+ local_time.tm_year + 1900, local_time.tm_mon + 1,
+ local_time.tm_mday, local_time.tm_hour,
+ local_time.tm_min, local_time.tm_sec);
+ }
+
+ const int offset_minutes = TimeZoneOffsetInMinutes(local_time, *utc_time);
+ const int abs_offset_minutes =
+ offset_minutes < 0 ? -offset_minutes : offset_minutes;
+ return ByteString::Format(
+ "D:%04d%02d%02d%02d%02d%02d%c%02d'%02d'", local_time.tm_year + 1900,
+ local_time.tm_mon + 1, local_time.tm_mday, local_time.tm_hour,
+ local_time.tm_min, local_time.tm_sec, offset_minutes < 0 ? '-' : '+',
+ abs_offset_minutes / kMinutesPerHour,
+ abs_offset_minutes % kMinutesPerHour);
+}
+
} // namespace
FPDF_EXPORT FPDF_DOCUMENT FPDF_CALLCONV FPDF_CreateNewDocument() {
@@ -185,9 +234,7 @@
if (FXSYS_time(¤tTime) != -1) {
tm* pTM = FXSYS_localtime(¤tTime);
if (pTM) {
- DateStr = ByteString::Format(
- "D:%04d%02d%02d%02d%02d%02d", pTM->tm_year + 1900, pTM->tm_mon + 1,
- pTM->tm_mday, pTM->tm_hour, pTM->tm_min, pTM->tm_sec);
+ DateStr = FormatPDFDate(currentTime, *pTM);
}
}
}
Change information
- M fpdfsdk/fpdf_edit_embeddertest.cpp
- M fpdfsdk/fpdf_editpage.cpp
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 7 comments![]( "Open in Gerrit")
Thanks for taking this on. Mostly questions about how to re-organize your work for modularization.
nit: blank line after this header but not before to group c/c++ includes into sections.
time_t FakeTime() {I'm pretty sure we've got a mechanism to set fake local time for other tests ... see fake_time_test.cpp. Maybe this can be moved there or otherwise re-used.
"0000000199 00000 n\r\n"I didn't see where 7 extra bytes were introduced prior to these size recalculations.
FSDK_SetTimeFunction(nullptr);Use fxcrt::AutoRestorer here if possible.
int64_t DaysSinceEpoch(const tm& time) {Because `local_time` and `utc_time` represent the exact same moment in time, their difference is bounded by the maximum timezone offsets (which range from -12 to +14 hours). This means the two timestamps will only ever be at most 1 day apart.
We don't need a full epoch conversion to find the difference in days. We can just compare `tm_mday`. If the difference is `> 1` or `< -1`, we know it wrapped around a month or year boundary and can safely map it to `-1` or `1`.
You can drop `DaysSinceEpoch()` entirely and simplify the calculation to:
```cpp
int TimeZoneOffsetInMinutes(const tm& local_time, const tm& utc_time) {
int day_offset = local_time.tm_mday - utc_time.tm_mday;
if (day_offset > 1) {
day_offset = -1;
} else if (day_offset < -1) {
day_offset = 1;
}
return day_offset * kMinutesPerDay +
(local_time.tm_hour - utc_time.tm_hour) * kMinutesPerHour +
(local_time.tm_min - utc_time.tm_min);
}
```
int TimeZoneOffsetInMinutes(const tm& local_time, const tm& utc_time) {Maybe this should go into fx_extension.{h,cpp} as FXSYS_TimeZoneOffsetMinutes() instead of here.
Related details
- Kuka
- Lei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kuka (Gerrit)
Kuka added 6 comments![]( "Open in Gerrit")
nit: blank line after this header but not before to group c/c++ includes into sections.
Done
I'm pretty sure we've got a mechanism to set fake local time for other tests ... see fake_time_test.cpp. Maybe this can be moved there or otherwise re-used.
Done. Reused pdfium::FakeTimeScope from testing/fake_time_test.h and removed the local FakeTime/FakeLocaltime helpers from this test.
I didn't see where 7 extra bytes were introduced prior to these size recalculations.
Done
Use fxcrt::AutoRestorer here if possible.
Done. Added scoped time-function helpers that use fxcrt::AutoRestorer internally, and updated the test to use them so the hooks are restored automatically.
Because `local_time` and `utc_time` represent the exact same moment in time, their difference is bounded by the maximum timezone offsets (which range from -12 to +14 hours). This means the two timestamps will only ever be at most 1 day apart.
We don't need a full epoch conversion to find the difference in days. We can just compare `tm_mday`. If the difference is `> 1` or `< -1`, we know it wrapped around a month or year boundary and can safely map it to `-1` or `1`.
You can drop `DaysSinceEpoch()` entirely and simplify the calculation to:
```cpp
int TimeZoneOffsetInMinutes(const tm& local_time, const tm& utc_time) {
int day_offset = local_time.tm_mday - utc_time.tm_mday;
if (day_offset > 1) {
day_offset = -1;
} else if (day_offset < -1) {
day_offset = 1;
}return day_offset * kMinutesPerDay +
(local_time.tm_hour - utc_time.tm_hour) * kMinutesPerHour +
(local_time.tm_min - utc_time.tm_min);
}
```
Done. Removed `DaysSinceEpoch()` and simplified the calculation to use the bounded `tm_mday` difference with month/year wrap handling.
int TimeZoneOffsetInMinutes(const tm& local_time, const tm& utc_time) {Maybe this should go into fx_extension.{h,cpp} as FXSYS_TimeZoneOffsetMinutes() instead of here.
Done. Moved the calculation to `FXSYS_TimeZoneOffsetInMinutes()` in `fx_extension.{h,cpp}` and added focused unit coverage there.
Related details
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez voted and added 3 comments![]( "Open in Gerrit")
Votes added by Tom Sepez
| Code-Review | +1 |
3 comments
Add a deterministic regression test for a non-hour timezone offset andThe folks on Lord Howe Island will appreciate this :) .
If you were so inclined, `using fxcrt::ScopedLocalTimeFunction;` since the namespace here is to avoid linker conflicts, but this would be the only such name in pdfium. Same for the other one.
tm local_time = {};
tm utc_time = {};
local_time.tm_mday = 23;
local_time.tm_hour = 10;
local_time.tm_min = 35;you can likely write these using designated initializers, e.g.
```
const tm local_time = {
.tm_mday = 23,
.tm_hour = 10,
.tm_min = 35.
};
Then surround each test case with
{
}
to re-declare and avoid assignments.
Related details
- Kuka
- Lei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kuka (Gerrit)
Kuka added 1 comment![]( "Open in Gerrit")
Done. Could you PTAL when you have a chance? Thank you!
Related details
- Kuka
- Lei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kuka (Gerrit)
Kuka added 3 comments![]( "Open in Gerrit")
Done. Addressed both comments in the latest patch set. Could you PTAL when you have a chance? Thank you!
If you were so inclined, `using fxcrt::ScopedLocalTimeFunction;` since the namespace here is to avoid linker conflicts, but this would be the only such name in pdfium. Same for the other one.
Done
tm local_time = {};
tm utc_time = {};
local_time.tm_mday = 23;
local_time.tm_hour = 10;
local_time.tm_min = 35;you can likely write these using designated initializers, e.g.
```
const tm local_time = {
.tm_mday = 23,
.tm_hour = 10,
.tm_min = 35.
};Then surround each test case with
{
}
to re-declare and avoid assignments.
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tom Sepez
| Code-Review | +1 |
Done. Addressed both comments in the latest patch set. Could you PTAL when you have a chance? Thank you!
- Kuka
- Lei Zhang
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
class ScopedTimeFunction {Since these scoper classes are only used in tests, can they move into testing/utils/ instead?
Kukanit: blank line after this header but not before to group c/c++ includes into sections.
Done
Still need to swap the 2 lines so the C headers are grouped together.
Related details
- Kuka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 3 comments![]( "Open in Gerrit")
ByteString FormatPDFDate(time_t current_time, tm local_time) {Take a const-ref?
offset_minutes < 0 ? -offset_minutes : offset_minutes;Just use abs()?
"D:%04d%02d%02d%02d%02d%02d%c%02d'%02d'", local_time.tm_year + 1900,Instead of repeating line 177 with the timezone offset, how about just use the same string and then append the TZ offset here?
Related details
- Kuka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kuka (Gerrit)
Kuka added 6 comments![]( "Open in Gerrit")
Thanks for the review. I addressed all comments in the latest patchset. PTAL when you have a chance.
Since these scoper classes are only used in tests, can they move into testing/utils/ instead?
Kukanit: blank line after this header but not before to group c/c++ includes into sections.
Lei ZhangDone
Still need to swap the 2 lines so the C headers are grouped together.
Done
ByteString FormatPDFDate(time_t current_time, tm local_time) {KukaTake a const-ref?
Done
offset_minutes < 0 ? -offset_minutes : offset_minutes;KukaJust use abs()?
Done
"D:%04d%02d%02d%02d%02d%02d%c%02d'%02d'", local_time.tm_year + 1900,Instead of repeating line 177 with the timezone offset, how about just use the same string and then append the TZ offset here?
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
using TimeFunction = time_t (*)();These used useful in the header in the earlier patchset. They improve readability.
#include "core/fxcrt/fx_extension.h"Only used in the .cc file, unless fx_extension.h defines TimeFunction/LocaltimeFunction.
Related details
- Kuka
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |