Created attachment 257972 [details] [review]
configureac.patch

Review of attachment 257971 [details] [review]:

I'm not against patches that make Gcr easier to use on Win32.

That said, I'm certainly not targetting Win32 in my development and addition of new features. Please keep in mind, that you will need to continue to provide fixes and adjustments if you wish to continue to make Gcr work on Win32.

I'm not a big fan of redefining broken versions (see below) of standard C functions inside of the code, especially when we have GLib around which cross-platform analogs for some of the things here. But if we have to have a replacement version of a funciton, it must behave identically to what it's replacing, nasty bugs have been caused (in projects like firefox) by cavalier porting efforts (ie: people defining strndup wrong, etc..)

So I've added a few notes, of things that need reworking before this patch can land.

All the best.

::: egg/egg-secure-memory.c
@@ +205,3 @@
+		pages = VirtualAlloc (NULL, (size_t)len, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
+		if (pages == NULL)
+#endif

Since Windows doesn't have mlock(), could you just disable usage of egg-secure-memory.c on WIN32? You can change the patch so NULL is unconditionally returned from sec_block_create() and the code above that in egg-secure-memory.c is not compiled on G_OS_WIN32. Hope that makes sense.

@@ +1847,3 @@
+gmtime_r (const time_t *const timep, struct tm *p_tm)
+{
+	static struct tm* tmp;

This is a completely non-threadsafe definition of gmtime_r. The whole point of gmtime_r is to be thread safe.

If cross platform compatibility is to be the goal, then we should probably just use g_date_set_time_t() as a gmtime_r replacement in egg-asn1x.c.

@@ +67,3 @@
+	const unsigned char *src = s + n;
+	unsigned char uc = c;
+	while (--src >= (unsigned char *) s)

For clarity, could you include braces around multiline loops or conditionals, thanks.

@@ +650,3 @@
+gmtime_r (const time_t *const timep, struct tm *p_tm)
+{
+	static struct tm *tmp;

Again, this is a completely non-threadsafe definition of gmtime_r.

A cross platform replacement in gck-slot.c would be to replace the gmtime_r/strftime calls with g_date_time_new_from_unix_utc/g_date_strftime.

@@ +664,3 @@
+#ifndef HAVE_TIMEGM
+time_t
+		memcpy (p_tm, tmp, sizeof(struct tm));

timegm is already available in this function via egg-timegm.h, not sure why it's necessary to duplicate that again.

@@ +707,3 @@
+
+const char *
+

We can migrate away from strptime, and use g_time_val_from_iso8601() which is more cross platform. We'll just need to delimit the incoming string appropriately with dashes spaces and colons before parsing.

@@ +46,3 @@
+#define WEXITSTATUS(x) ((x) & 0xff)
+#define WIFSIGNALED(x) ((unsigned)(x) > 259)
+#define WTERMSIG(x) SIGTERM

Do these definitions even apply to Windows? Do they work?

@@ +889,3 @@
+
+const char *
+			break;

You can use g_time_val_from_iso8601() in gcr-record as a cross platform compatible parser, although you'll need to use a buffer to fill in zeros for the hour/minute/second.

@@ +50,3 @@
+extern void errx (int eval, char *format, ...);
+
+void errx (int eval, char *format, ...)

This is just sample code. Please migrate it away from errx instead of providing this version which behaves completely different than errx() usually does.

(In reply to comment #2)
Hello Stef, and many thanks for your review,

> I'm not against patches that make Gcr easier to use on Win32.
> 
> That said, I'm certainly not targetting Win32 in my development and addition of
> new features. Please keep in mind, that you will need to continue to provide
> fixes and adjustments if you wish to continue to make Gcr work on Win32.

Fair enough. I will periodically need to do upgrades, so I'll update the code accordingly.

> I'm not a big fan of redefining broken versions (see below) of standard C
> functions inside of the code, especially when we have GLib around which
> cross-platform analogs for some of the things here. But if we have to have a
> replacement version of a funciton, it must behave identically 

I totally agree.
As I wasn't sure my patch wasn't going to be accepted (or even reviewed), I made a version that "just worked" without enforcing 100% accuracy everywhere.  Now that I know that there's someone listening (thx again), I'll be careful to provide better implementations.

> ::: egg/egg-secure-memory.c

> Since Windows doesn't have mlock(), could you just disable usage of
> egg-secure-memory.c on WIN32? You can change the patch so NULL is
> unconditionally returned from sec_block_create() and the code above that in
> egg-secure-memory.c is not compiled on G_OS_WIN32. Hope that makes sense.

It does ! Will do that.

> 
> This is a completely non-threadsafe definition of gmtime_r. The whole point of
> gmtime_r is to be thread safe.
> 
> If cross platform compatibility is to be the goal, then we should probably just
> use g_date_set_time_t() as a gmtime_r replacement in egg-asn1x.c.
> 
> Again, this is a completely non-threadsafe definition of gmtime_r.
> 
> A cross platform replacement in gck-slot.c would be to replace the
> gmtime_r/strftime calls with g_date_time_new_from_unix_utc/g_date_strftime.
> 

Understood, good information here. Will experiment with these GLib functions and try to incorporate them.

> timegm is already available in this function via egg-timegm.h, not sure why
> it's necessary to duplicate that again.

My mistake.

> We can migrate away from strptime, and use g_time_val_from_iso8601() which is
> more cross platform. We'll just need to delimit the incoming string
> appropriately with dashes spaces and colons before parsing.
> 
> You can use g_time_val_from_iso8601() in gcr-record as a cross platform
> compatible parser, although you'll need to use a buffer to fill in zeros for
> the hour/minute/second.

Useful information here again, very nice, will try that.

> 
> This is just sample code. Please migrate it away from errx instead of providing
> this version which behaves completely different than errx() usually does.

No problem.


I'll work on this and try to provide a better patch during the next week.

Created attachment 260403 [details] [review]
gcr362-win32.patch

Hello, as promised, but with a little delay due to both priorities and wanting to get this fully right : new version of the patch following Stef's review suggestions.

Here are some details :

+#if defined(HAVE_MLOCK)
 	/* Make sure sz is a multiple of the page size */
 	pgsize = getpagesize ();
 	*sz = (*sz + pgsize -1) & ~(pgsize - 1);

put all this into #ifdef MLOCK, as it fixes Win32 which it what we want, and the function would be pointless without it anyway.

+#ifdef _WIN32
+	/* win32 does not have mlock(), so just fail in that case */
+	return NULL;
+#endif

totally disable secure memory on Win32. Effectively, tests such as "egg/tests/test-secmem" fail as intended after that.

+void*
+memrchr (const void *s, int c, size_t n)
+{

New implementation of the replacement function, hope it's clearer this way.

-		if (!strptime (string, "%Y%m%d%H%M%S", &tm))
+                /* Transform into an ISO-8601 string */
+		string = g_strconcat (g_strndup (string,8), "T", g_strndup (string+8,6), "Z", NULL);
+		if (!g_time_val_from_iso8601 (string, &tv))

I noticed "g_time_val_from_iso8601()" accepted "%Y%m%d T %H%M%S" syntax ; if there's a problem with that, we can still add "Z" at the end of the string.

+#ifndef G_OS_UNIX
+#define WIFEXITED(x) 1
+#define WEXITSTATUS(x) (x)
+#define WIFSIGNALED(x) 0
+#define WTERMSIG(x) (x)
+#endif

We have no way to catch signals on Win32, so always suppose it has exited and get the return value.

 static void
 on_gnupg_process_child_setup (gpointer user_data)
 {
+#ifdef G_OS_UNIX 

Child function is never executed on Win32 ; it only seemed to deal with FD_CLOEXEC flags which don't exist on this platform anyway, so I guess it's no big deal.

+#if defined(GDK_WINDOWING_X11)
 		gdk_window_set_transient_for (window, transient_for);
+#elif defined(GDK_WINDOWING_WIN32)
+		HWND chandle = gdk_win32_window_get_handle (window);
+		SetWindowLongPtr (chandle, GWLP_HWNDPARENT, (LONG_PTR)handle);

"gdk_window_set_transient_for()" tends to fail on Win32 when concerned GdkWindows don't belong to the same thread or process. Using lower-level API gives better results.

+#ifndef HAVE_ERR_H
+extern void errx (int eval, char *format, ...);
+
+void errx (int eval, char *format, ...)
+{

Better replacement function.

Linux still builds and seems to works OK after the patch.

Created attachment 260404 [details] [review]
configureac.patch

Hi, any news on this ?
Thanks.

Created attachment 283122 [details] [review]
patch to compile version 3.12.2 under win32

i updated the patch to work with version 3.12.2

i intentionally removed some piece of code because i didn't know how to fix the problem

Review of attachment 260404 [details] [review]:

::: configure.ac
@@ +68,3 @@
+
+if test "$os_unix" = "yes"; then
+	GDK_GIO_PACKAGE=gio-2.0 gio-unix-2.0

This is not Gdk. So the 'GDK_' prefix can be removed from this variable.

Review of attachment 260403 [details] [review]:

In principle the changes are good. Some comments on things that need to be fixed before bringing them into the project.

In addition these need to be put into proper git commits. The patches do not currently apply to master, so they would need to replaced.

::: egg/egg-secure-memory.c
@@ +635,3 @@
 		string = g_strndup ((gchar*)info->utcTime, MIN (14, sizeof (info->utcTime)));
+                /* Transform into an ISO-8601 string */
+		string = g_strconcat (g_strndup (string,8), "T", g_strndup (string+8,6), "Z", NULL);

This is a leak here. string is allocated twice in a row and freed once.

@@ +696,2 @@
 			g_return_if_reached ();
+		buffer = g_date_time_format (datetime, "%Y%m%d%H%M%S");

Seems like buffer leaks.

@@ +911,3 @@
+
+#ifdef G_OS_WIN32
+	g_spawn_close_pid (pid);

This does not need an ifdef. The code is valid on unix too.

@@ +1034,3 @@
 		if (pipe (status_fds) < 0)
+#elif defined(G_OS_WIN32)
+		if (_pipe (status_fds, 4096, _O_BINARY) < 0)

Could you make a compatibility pipe() function?

@@ +1080,2 @@
 		g_free (command);
+		g_free (environment);

Could you commit this as a separate patch? It seems to be an enhancement.

@@ +59,3 @@
+	va_end (ap);
+
+	gchar *err_s;

The value returned from g_strdup() leaks.

Review of attachment 257972 [details] [review]:

In general this is sound. But this needs to be built into a proper git commit that applies to the master branch.

::: configure.ac
@@ +68,3 @@
+
+if test "$os_unix" = "yes"; then
+	GDK_GIO_PACKAGE=gio-2.0 gio-unix-2.0

This breaks the build on unixy OS's. You seem to be missing quotes.

In addition please remove the 'GDK_' prefix for this variable. The packages in quetsion have nothing to do with Gdk

Review of attachment 283122 [details] [review]:

This patch needs the fixes that I've noted above (on the separate patches). In addition it needs to be rebased on git master, and built into proper git commits before we can merge it.

::: gcr-3.12.2-orig/configure.ac
@@ +275,2 @@
 	test-simple-certificate \
 	test-certificate \

Sorry, we can't just remove tests.

@@ +37,2 @@
 	$(NULL)
 

Once again, we should be disabling code on WIN32, not just flat out removing code.

Review of attachment 283122 [details] [review]:

Needs work.

Given that there has been no update on this for >5 years, we can close this issue. Feel free to open a Merge Request in gcr's GitLab repository if you want to have updated patches reviewed.