Timestamp support by TheComputerGuy96 · Pull Request #187 · twogood/unshield

TheComputerGuy96 · 2024-12-14T17:44:54Z

Fixes #100

The timestamp setting code currently doesn't use the file wrappers though (and is directly located in unshield.c)

twogood · 2024-12-14T18:03:25Z

Very nice! What work is need to support it in the wrapper?

TheComputerGuy96 · 2024-12-14T18:52:00Z

What work is need to support it in the wrapper?

I think it's as easy as adding the relevant callbacks (but I'm not sure if those 2 functions have alternative implementations; especially the utime() function)

These were originally present in the early unshield code but for some reason they were removed later (this change is based on the i5comp and i6comp struct layout) Those struct members have a correct date with the Baldur's Gate test (likely <IS6?) and GTA San Andreas (IS9)

TheComputerGuy96 · 2026-01-09T14:58:39Z

After looking some more, I noticed the unshield binary uses direct stat() calls (so function wrappers may not be needed there)

Also while the timestamp setting code could be moved to another function or as a part of common extract code, I don't think it's that important

twogood · 2026-01-09T15:05:50Z

After looking some more, I noticed the unshield binary uses direct stat() calls (so function wrappers may not be needed there)

Also while the timestamp setting code could be moved to another function or as a part of common extract code, I don't think it's that important

I thought of including it in function wrappers for consistency.

Copilot

Pull request overview

This pull request adds timestamp support to the unshield tool, allowing users to preserve the original modification date/time from InstallShield metadata when extracting files. This addresses issue #100.

Key Changes:

Adds a new -t command-line flag to enable timestamp preservation during extraction
Implements DOS timestamp parsing and conversion to Unix time_t
Updates file listing to display timestamps in ISO 8601 format

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/unshield.c	Adds `-t` option handling, timestamp setting logic in extract_file(), and date display in file listing
man/unshield.1	Documents the new `-t` option and updates man page date to December 2024
lib/libunshield.h	Declares two new public API functions: unshield_file_iso_date() and unshield_file_timestamp()
lib/internal.h	Declares internal helper function unshield_dos_to_tm() for DOS timestamp conversion
lib/helper.c	Implements unshield_dos_to_tm() to convert DOS date/time format to struct tm
lib/file.c	Implements the new API functions and adds DOS date/time parsing in file descriptor reading
lib/cabfile.h	Adds dos_date and dos_time fields to FileDescriptor structure

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

man/unshield.1

src/unshield.c

lib/helper.c

src/unshield.c

lib/file.c

twogood · 2026-01-11T14:53:44Z

@TheComputerGuy96 not meaning to give you extra work but do you think the Copilot comments have any merit? It could also be possible to merge this PR and address those comments in a second PR.

This also adds a helper function to load the DOS date/time into a tm structure and an library function to get the ISO 8601 date Co-authored-by: GitHub Copilot <copilot@github.com>

This also adds an API function for getting an Unix timestamp (and updates the man page) Co-authored-by: GitHub Copilot <copilot@github.com>

TheComputerGuy96 · 2026-01-14T10:54:42Z

do you think the Copilot comments have any merit?

@twogood I added extra safety checks in the new PR version (which should mostly appease Copilot)

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T21:15:35Z

lib/file.c

+      fd->dos_date          = (uint16_t)READ_UINT32(p); p += 4;
+      fd->dos_time          = (uint16_t)READ_UINT32(p); p += 4;
+      p += 8;


The code reads dos_date and dos_time as 32-bit values using READ_UINT32 and then casts them to 16-bit. This seems inconsistent with the later versions (6+) which read them as 16-bit values directly. If the file format actually stores these as 32-bit values in versions 0 and 5, consider documenting why this is the case. If they are actually 16-bit fields, use READ_UINT16 instead and adjust the pointer increment accordingly. The current approach may work but could be confusing and potentially read incorrect data depending on endianness and padding.

Copilot · 2026-01-21T21:15:36Z

src/unshield.c

  dirname = malloc(path_max);
-  filename = malloc(path_max);
+  full_filename = malloc(path_max);
  if (real_output_directory == NULL || real_filename == NULL)


The NULL check only verifies real_output_directory and real_filename, but doesn't check the newly allocated dirname and full_filename pointers. If malloc fails for these variables, they will be NULL and dereferencing them later will cause a segmentation fault. The NULL check should include dirname and full_filename as well.

Suggested change

if (real_output_directory == NULL || real_filename == NULL)

if (real_output_directory == NULL || real_filename == NULL ||

dirname == NULL || full_filename == NULL)

Copilot · 2026-01-21T21:15:36Z

src/unshield.c

+      fprintf(stderr, "Failed to get timestamp for file '%s'.", filename);
+    }
+    else
+    {
+      memset(&utim, 0, sizeof(utim));
+      memset(&info, 0, sizeof(info));
+
+      ret = stat(full_filename, &info);
+      if (ret == 0)
+      {
+        utim.actime = info.st_atime;
+        utim.modtime = timestamp;
+        ret2 = utime(full_filename, &utim);
+      }
+
+      if (ret < 0 || ret2 < 0)
+        fprintf(stderr, "Failed to set timestamp for file '%s'.", filename);


The error messages on lines 561 and 577 are missing a newline character at the end, which is inconsistent with other error messages in this file and can make the output harder to read. Add '\n' at the end of both error messages.

Copilot · 2026-01-21T21:15:36Z

src/unshield.c

+    if (!timestamp)
+    {
+      fprintf(stderr, "Failed to get timestamp for file '%s'.", filename);
+    }
+    else
+    {
+      memset(&utim, 0, sizeof(utim));
+      memset(&info, 0, sizeof(info));
+
+      ret = stat(full_filename, &info);
+      if (ret == 0)
+      {
+        utim.actime = info.st_atime;
+        utim.modtime = timestamp;
+        ret2 = utime(full_filename, &utim);
+      }
+
+      if (ret < 0 || ret2 < 0)
+        fprintf(stderr, "Failed to set timestamp for file '%s'.", filename);
+    }


The check for a zero timestamp may not be appropriate here. A timestamp of 0 (Unix epoch: January 1, 1970) is a valid timestamp value, though unlikely for InstallShield files. The code should distinguish between a valid timestamp of 0 and an error condition. Consider checking if unshield_file_timestamp returned 0 due to an error versus having an actual 0 timestamp (which would be a valid DOS date/time of 00:00:00 on an invalid date). The current implementation will skip setting timestamps for files that legitimately have a timestamp of 0.

Suggested change

if (!timestamp)

{

fprintf(stderr, "Failed to get timestamp for file '%s'.", filename);

}

else

{

memset(&utim, 0, sizeof(utim));

memset(&info, 0, sizeof(info));

ret = stat(full_filename, &info);

if (ret == 0)

{

utim.actime = info.st_atime;

utim.modtime = timestamp;

ret2 = utime(full_filename, &utim);

}

if (ret < 0 || ret2 < 0)

fprintf(stderr, "Failed to set timestamp for file '%s'.", filename);

}

memset(&utim, 0, sizeof(utim));

memset(&info, 0, sizeof(info));

ret = stat(full_filename, &info);

if (ret == 0)

{

utim.actime = info.st_atime;

utim.modtime = timestamp;

ret2 = utime(full_filename, &utim);

}

if (ret < 0 || ret2 < 0)

fprintf(stderr, "Failed to set timestamp for file '%s'.", filename);

Copilot · 2026-01-21T21:15:37Z

src/unshield.c

+    struct utimbuf utim;
+    struct stat info;
+
+    if (!timestamp)
+    {
+      fprintf(stderr, "Failed to get timestamp for file '%s'.", filename);
+    }
+    else
+    {
+      memset(&utim, 0, sizeof(utim));
+      memset(&info, 0, sizeof(info));
+
+      ret = stat(full_filename, &info);
+      if (ret == 0)
+      {
+        utim.actime = info.st_atime;
+        utim.modtime = timestamp;
+        ret2 = utime(full_filename, &utim);


The code uses POSIX utime() and struct utimbuf without Windows-specific handling. On Windows, these may not be available or may need to use _utime() and struct _utimbuf instead. Consider adding platform-specific code or using a more portable approach like utimensat/futimens on systems that support them, or adding Windows-specific equivalents with conditional compilation.

Copilot · 2026-01-21T21:15:37Z

src/unshield.c

+  if (success && set_timestamp)
+  {
+    time_t timestamp = unshield_file_timestamp(unshield, index);
+    int ret = 0, ret2 = 0;
+    struct utimbuf utim;
+    struct stat info;
+
+    if (!timestamp)
+    {
+      fprintf(stderr, "Failed to get timestamp for file '%s'.", filename);
+    }
+    else
+    {
+      memset(&utim, 0, sizeof(utim));
+      memset(&info, 0, sizeof(info));
+
+      ret = stat(full_filename, &info);
+      if (ret == 0)
+      {
+        utim.actime = info.st_atime;
+        utim.modtime = timestamp;
+        ret2 = utime(full_filename, &utim);
+      }
+
+      if (ret < 0 || ret2 < 0)
+        fprintf(stderr, "Failed to set timestamp for file '%s'.", filename);
+    }
+  }


The new timestamp setting functionality (-t flag) lacks test coverage. Consider adding tests that verify: 1) timestamps are correctly set when the -t flag is used, 2) extraction still works when timestamps cannot be set, 3) the timestamp values match expected DOS date/time conversions. This would help ensure the feature works correctly across different platforms and edge cases.

twogood self-assigned this Dec 14, 2024

TheComputerGuy96 force-pushed the timestamp-support branch 2 times, most recently from 5a67093 to b3c5979 Compare January 9, 2026 14:57

twogood requested a review from Copilot January 9, 2026 15:06

Copilot started reviewing on behalf of twogood January 9, 2026 15:06 View session

Copilot AI reviewed Jan 9, 2026

View reviewed changes

man/unshield.1 Outdated Show resolved Hide resolved

src/unshield.c Outdated Show resolved Hide resolved

lib/helper.c Show resolved Hide resolved

src/unshield.c Outdated Show resolved Hide resolved

lib/file.c Show resolved Hide resolved

TheComputerGuy96 and others added 2 commits January 14, 2026 12:07

Print modification date in file listings

c553961

This also adds a helper function to load the DOS date/time into a tm structure and an library function to get the ISO 8601 date Co-authored-by: GitHub Copilot <copilot@github.com>

Add option to set the modification date/time

a83e977

This also adds an API function for getting an Unix timestamp (and updates the man page) Co-authored-by: GitHub Copilot <copilot@github.com>

TheComputerGuy96 force-pushed the timestamp-support branch from b3c5979 to a83e977 Compare January 14, 2026 10:52

twogood requested a review from Copilot January 21, 2026 21:09

Copilot started reviewing on behalf of twogood January 21, 2026 21:10 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

	if (real_output_directory == NULL \|\| real_filename == NULL)
	if (real_output_directory == NULL \|\| real_filename == NULL \|\|
	dirname == NULL \|\| full_filename == NULL)

Conversation

TheComputerGuy96 commented Dec 14, 2024

Uh oh!

twogood commented Dec 14, 2024

Uh oh!

TheComputerGuy96 commented Dec 14, 2024

Uh oh!

TheComputerGuy96 commented Jan 9, 2026

Uh oh!

twogood commented Jan 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

twogood commented Jan 11, 2026

Uh oh!

TheComputerGuy96 commented Jan 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants