Windows port minor fixes
Hi,
According to microsoft documentation at:
https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom
The function CryptGenRandom is deprecated, and may can be removed in future release.
Considering that postgres only supports windows versions that have the new API, it would be good to make the replace.
BCryptGenRandom apparently works without having to set up an environment before calling it, allowing a simplification in the file that makes the call.
The drawback, its change causes need to link to bcrypt.lib.
On exec.c, have two memory leaks, and a possible access beyond heap bounds, the patch tries to fix them.
According to documentation at:
https://en.cppreference.com/w/c/experimental/dynamic/strdup
"The returned pointer must be passed to free to avoid a memory leak. "
* memory leak fix to src/common/exec.c
* CryptGenRandom change by BCryptGenRandom to src/port/pg_strong_random.c
* link bcrypt.lib to src/tools/msvc/Mkvcbuild.pm
regards,
Ranier Vilela
Attachments:
exec_v1.patchtext/x-patch; name=exec_v1.patchDownload
diff --git a/src/common/exec.c b/src/common/exec.c
index 92dc3134a1..88a27cec78 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -72,14 +72,15 @@ validate_exec(const char *path)
int is_x;
#ifdef WIN32
- char path_exe[MAXPGPATH + sizeof(".exe") - 1];
+ char path_exe[MAXPGPATH + sizeof(".exe")];
+ int path_len;
/* Win32 requires a .exe suffix for stat() */
- if (strlen(path) >= strlen(".exe") &&
- pg_strcasecmp(path + strlen(path) - strlen(".exe"), ".exe") != 0)
+ path_len = strlen(path);
+ if (path_len >= (sizeof(".exe") - 1) &&
+ pg_strcasecmp(path + path_len - (sizeof(".exe") - 1), ".exe") != 0)
{
- strlcpy(path_exe, path, sizeof(path_exe) - 4);
- strcat(path_exe, ".exe");
+ snprintf(path_exe, sizeof(path_exe) - 5, "%s.exe", path);
path = path_exe;
}
#endif
@@ -600,8 +601,10 @@ set_pglocale_pgservice(const char *argv0, const char *app)
snprintf(env_path, sizeof(env_path), "PGLOCALEDIR=%s", path);
canonicalize_path(env_path + 12);
dup_path = strdup(env_path);
- if (dup_path)
- putenv(dup_path);
+ if (dup_path) {
+ putenv(dup_path);
+ free(dup_path);
+ }
}
#endif
@@ -613,8 +616,10 @@ set_pglocale_pgservice(const char *argv0, const char *app)
snprintf(env_path, sizeof(env_path), "PGSYSCONFDIR=%s", path);
canonicalize_path(env_path + 13);
dup_path = strdup(env_path);
- if (dup_path)
- putenv(dup_path);
+ if (dup_path) {
+ putenv(dup_path);
+ free(dup_path);
+ }
}
}
pg_strong_random_v1.patchtext/x-patch; name=pg_strong_random_v1.patchDownload
diff --git a/src/port/pg_strong_random.c b/src/port/pg_strong_random.c
index 6be5874cbf..6d075eaf1a 100644
--- a/src/port/pg_strong_random.c
+++ b/src/port/pg_strong_random.c
@@ -28,17 +28,13 @@
#include <openssl/rand.h>
#endif
#ifdef USE_WIN32_RANDOM
-#include <wincrypt.h>
+#include <bcrypt.h>
+#ifndef STATUS_SUCCESS
+#define STATUS_SUCCESS ((NTSTATUS)0x00000000L)
#endif
-
-#ifdef USE_WIN32_RANDOM
-/*
- * Cache a global crypto provider that only gets freed when the process
- * exits, in case we need random numbers more than once.
- */
-static HCRYPTPROV hProvider = 0;
#endif
+
#if defined(USE_DEV_URANDOM)
/*
* Read (random) bytes from a file.
@@ -85,7 +81,7 @@ random_from_file(const char *filename, void *buf, size_t len)
* We support a number of sources:
*
* 1. OpenSSL's RAND_bytes()
- * 2. Windows' CryptGenRandom() function
+ * 2. Windows' BCryptGenRandom() function
* 3. /dev/urandom
*
* The configure script will choose which one to use, and set
@@ -140,28 +136,8 @@ pg_strong_random(void *buf, size_t len)
* Windows has CryptoAPI for strong cryptographic numbers.
*/
#elif defined(USE_WIN32_RANDOM)
- if (hProvider == 0)
- {
- if (!CryptAcquireContext(&hProvider,
- NULL,
- MS_DEF_PROV,
- PROV_RSA_FULL,
- CRYPT_VERIFYCONTEXT | CRYPT_SILENT))
- {
- /*
- * On failure, set back to 0 in case the value was for some reason
- * modified.
- */
- hProvider = 0;
- }
- }
- /* Re-check in case we just retrieved the provider */
- if (hProvider != 0)
- {
- if (CryptGenRandom(hProvider, len, buf))
- return true;
- }
- return false;
+ return (BCryptGenRandom(NULL, buf, len,
+ BCRYPT_USE_SYSTEM_PREFERRED_RNG) == STATUS_SUCCESS);
/*
* Read /dev/urandom ourselves.
Mkvcbuild_v1.patchtext/x-patch; name=Mkvcbuild_v1.patchDownload
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 275f3bb699..9e1ac51507 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -65,7 +65,7 @@ my @frontend_uselibpgcommon = (
my $frontend_extralibs = {
'initdb' => ['ws2_32.lib'],
'pg_restore' => ['ws2_32.lib'],
- 'pgbench' => ['ws2_32.lib'],
+ 'pgbench' => ['ws2_32.lib', 'bcrypt.lib'],
'psql' => ['ws2_32.lib']
};
my $frontend_extraincludes = {
@@ -184,6 +184,7 @@ sub mkvcbuild
$postgres->AddFiles('src/backend/utils/adt', 'jsonpath_scan.l',
'jsonpath_gram.y');
$postgres->AddDefine('BUILDING_DLL');
+ $postgres->AddLibrary('bcrypt.lib');
$postgres->AddLibrary('secur32.lib');
$postgres->AddLibrary('ws2_32.lib');
$postgres->AddLibrary('wldap32.lib') if ($solution->{options}->{ldap});
@@ -246,6 +247,7 @@ sub mkvcbuild
$libpq->AddDefine('FRONTEND');
$libpq->AddDefine('UNSAFE_STAT_OK');
$libpq->AddIncludeDir('src/port');
+ $libpq->AddLibrary('bcrypt.lib');
$libpq->AddLibrary('secur32.lib');
$libpq->AddLibrary('ws2_32.lib');
$libpq->AddLibrary('wldap32.lib') if ($solution->{options}->{ldap});
On Mon, Dec 16, 2019 at 6:34 PM Ranier Vilela <ranier_gyn@hotmail.com>
wrote:
Considering that postgres only supports windows versions that have the new
API, it would be good to make the replace.
That is not actually the case. If you check the _WIN32_WINNT logic
in src/include/port/win32.h you can see that depending on your building
tools you can get a version lower than that, for example if using MinGW.
* memory leak fix to src/common/exec.c
* CryptGenRandom change by BCryptGenRandom to src/port/pg_strong_random.c
* link bcrypt.lib to src/tools/msvc/Mkvcbuild.pm
If you want to address 2 unrelated issues, it makes little sense to use a
single thread and 3 patches.
Regards,
Juan José Santamaría Flecha
On Mon, Dec 16, 2019 at 07:57:10PM +0100, Juan José Santamaría Flecha wrote:
If you want to address 2 unrelated issues, it makes little sense to use a
single thread and 3 patches.
And if you actually group things together so as any individual looking
at your patches does not have to figure out which piece applies to
what, that's also better. Anyway, the patch for putenv() is wrong in
the way the memory is freed, but this has been mentioned on another
thread. We rely on MAXPGPATH heavily so your patch trying to change
the buffer length does not bring much, and the windows-crypt call is
also wrong based for the version handling, as discussed on another
thread.
--
Michael
De: Michael Paquier
Enviadas: Terça-feira, 17 de Dezembro de 2019 04:45
And if you actually group things together so as any individual looking
at your patches does not have to figure out which piece applies to
what, that's also better.
I'm still trying to find the best way.
Anyway, the patch for putenv() is wrong in the way the memory is freed, but this >has been mentioned on another thread.
Oh yes, putenv depending on glibc version, copy and not others, the pointer.
At Windows side, the Dr.Memory, reported two memory leaks, with strdup.
The v2 is better, because, simplifies the function.
Submitted a proposal for setenv support for Windows, in other thread.
We rely on MAXPGPATH heavily so your patch trying to change
the buffer length does not bring much,
I am a little confused about which path you are talking about.
If it about var path at function validate_exec, I believe that there is a mistake.
char path_exe[MAXPGPATH + sizeof(".exe") - 1];
The -1, its suspicious and can be removed.
Once there, I tried to improve the code by simplifying and removing the excessive number of functions.
At Windows side, the code paths, is less tested.
The Dr.Memory, reported 3794 potential unaddressable access at WIN32 block, pipe_read_line function, wich call validade_exec.
Best regards,
Ranier Vilela
Import Notes
Resolved by subject fallback
On Tue, Dec 17, 2019 at 9:06 AM Ranier Vilela <ranier_gyn@hotmail.com> wrote:
De: Michael Paquier
Enviadas: Terça-feira, 17 de Dezembro de 2019 04:45And if you actually group things together so as any individual looking
at your patches does not have to figure out which piece applies to
what, that's also better.I'm still trying to find the best way.
A lot of your emails, like this one, seem to be replies to other
emails, but at least in my mail reader (gmail) something you're doing
is causing the threading to get broken, so it's very hard to know what
this is replying to.
Also, the way the quoted material is presented in your emails is quite
odd-looking.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Greetings,
* Robert Haas (robertmhaas@gmail.com) wrote:
On Tue, Dec 17, 2019 at 9:06 AM Ranier Vilela <ranier_gyn@hotmail.com> wrote:
De: Michael Paquier
Enviadas: Terça-feira, 17 de Dezembro de 2019 04:45And if you actually group things together so as any individual looking
at your patches does not have to figure out which piece applies to
what, that's also better.I'm still trying to find the best way.
A lot of your emails, like this one, seem to be replies to other
emails, but at least in my mail reader (gmail) something you're doing
is causing the threading to get broken, so it's very hard to know what
this is replying to.
I'm reasonably confident (though I can't be sure about gmail, but I see
the same thing in mutt) the issue here is that there's no References or
In-Reply-To headers in the emails. There's some 'Thread-Subject' and
'Thread-Index' headers but it seems that gmail and mutt can't sort out
what those are or how to use them to do proper threading (if it's even
possible with those headers.. I'm not really sure how you'd use the
'Thread-Index' value since it seems to just be a hex code..).
Also, the way the quoted material is presented in your emails is quite
odd-looking.
Yeah, agree with this too, though that bothers me somewhat less than the
threading issue.
Thanks,
Stephen
De: Robert Haas <robertmhaas@gmail.com>
Enviado: quarta-feira, 18 de dezembro de 2019 15:44
A lot of your emails, like this one, seem to be replies to other
emails, but at least in my mail reader (gmail) something you're doing
is causing the threading to get broken, so it's very hard to know what
this is replying to.
I can't tell if it's me doing something wrong or if live outlook can't organize it the right way.
Anyway, I will switch to gmail,
ranier.vf@gmail.com, to see if it looks better.
regards,
Ranier Vilela
Greetings,
* Ranier Vilela (ranier_gyn@hotmail.com) wrote:
De: Robert Haas <robertmhaas@gmail.com>
Enviado: quarta-feira, 18 de dezembro de 2019 15:44A lot of your emails, like this one, seem to be replies to other
emails, but at least in my mail reader (gmail) something you're doing
is causing the threading to get broken, so it's very hard to know what
this is replying to.I can't tell if it's me doing something wrong or if live outlook can't organize it the right way.
Alright, well, oddly enough, *this* email included the other headers and
appears threaded properly (in mutt, at least).
Did you do something different when replying to this email vs. the other
emails you've been replying to?
Also- it's custom here to "reply-all" and not to just reply to the list.
Thanks,
Stephen
Em qua., 18 de dez. de 2019 às 15:59, Stephen Frost <sfrost@snowman.net>
escreveu:
Alright, well, oddly enough, *this* email included the other headers and
appears threaded properly (in mutt, at least).Did you do something different when replying to this email vs. the other
emails you've been replying to?Well, live outlook, is kinda dumb at that point, when responds, he add
only person email, not list email.
So the answer goes only to the person.
Also- it's custom here to "reply-all" and not to just reply to the list.
Ok, adjusted.
regards,
Ranier Vilela