Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)
Hi,
The pg_leftmost_one_pos32 function with msvc compiler,
relies on Assert to guarantee the correct result.
But msvc documentation [1]https://learn.microsoft.com/en-us/cpp/intrinsics/bitscanreverse-bitscanreverse64?view=msvc-170 says clearly that
undefined behavior can occur.
Fix by checking the result of the function to avoid
a possible undefined behavior.
patch attached.
best regards,
Ranier Vilela
[1]: https://learn.microsoft.com/en-us/cpp/intrinsics/bitscanreverse-bitscanreverse64?view=msvc-170
https://learn.microsoft.com/en-us/cpp/intrinsics/bitscanreverse-bitscanreverse64?view=msvc-170
Attachments:
0001-Avoid-undefined-behavior-with-msvc-compiler.patchapplication/octet-stream; name=0001-Avoid-undefined-behavior-with-msvc-compiler.patchDownload
From 8b7b159bcc39fe17f1dab8a9692f73694e1d9a70 Mon Sep 17 00:00:00 2001
From: Ranier Vilela <ranier.vf@gmail.com>
Date: Sat, 29 Jul 2023 09:17:50 -0300
Subject: [PATCH] Avoid undefined behavior with msvc compiler. [1] msvc
documentation says: [out] Loaded with the bit position of the first set bit
(1) found. Otherwise, undefined.
_BitScanReverse and _BitScanReverse64 really need check return value.
Because both can functions fails in search of bit index.
More with msvc compiler the current implementation lacks assertion checks
the input parameter.
Author: Ranier Vilela (ranier.vf@gmail.com)
[1] https://learn.microsoft.com/en-us/cpp/intrinsics/bitscanreverse-bitscanreverse64?view=msvc-170
---
src/include/port/pg_bitutils.h | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/src/include/port/pg_bitutils.h b/src/include/port/pg_bitutils.h
index 21a4fa0341..d285fb83ec 100644
--- a/src/include/port/pg_bitutils.h
+++ b/src/include/port/pg_bitutils.h
@@ -46,11 +46,15 @@ pg_leftmost_one_pos32(uint32 word)
return 31 - __builtin_clz(word);
#elif defined(_MSC_VER)
unsigned long result;
- bool non_zero;
+ unsigned char is_non_zero;
- non_zero = _BitScanReverse(&result, word);
- Assert(non_zero);
- return (int) result;
+ Assert(word != 0);
+
+ is_non_zero = _BitScanReverse(&result, word);
+ if (is_non_zero)
+ return (int) result;
+ else
+ return 0;
#else
int shift = 32 - 8;
@@ -83,11 +87,15 @@ pg_leftmost_one_pos64(uint64 word)
#elif defined(_MSC_VER) && (defined(_M_AMD64) || defined(_M_ARM64))
unsigned long result;
- bool non_zero;
+ unsigned char is_non_zero;
- non_zero = _BitScanReverse64(&result, word);
- Assert(non_zero);
- return (int) result;
+ Assert(word != 0);
+
+ is_non_zero = _BitScanReverse64(&result, word);
+ if (is_non_zero)
+ return (int) result;
+ else
+ return 0;
#else
int shift = 64 - 8;
--
2.32.0.windows.1
On Sat, Jul 29, 2023 at 7:37 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
Hi,
The pg_leftmost_one_pos32 function with msvc compiler,
relies on Assert to guarantee the correct result.But msvc documentation [1] says clearly that
undefined behavior can occur.
It seems that we should have "Assert(word != 0);" at the top, which matches
the other platforms anyway, so I'll add that.
Fix by checking the result of the function to avoid
a possible undefined behavior.
No other platform does this, and that is by design. I'm also not
particularly impressed by the unrelated cosmetic changes.
--
John Naylor
EDB: http://www.enterprisedb.com
John Naylor <john.naylor@enterprisedb.com> writes:
It seems that we should have "Assert(word != 0);" at the top, which matches
the other platforms anyway, so I'll add that.
That's basically equivalent to the existing Assert(non_zero).
I think it'd be okay to drop that one and instead have
the same Assert condition as other platforms, but having both
would be redundant.
I agree that adding the non-Assert test that Ranier wants
is entirely pointless. If a caller did manage to violate
the asserted-for condition (and we don't have asserts on),
returning zero is not better than returning an unspecified
value. If anything it might be worse, since it might not
lead to obvious misbehavior.
On the whole, there's not anything wrong with the code as-is.
A case could be made for making the MSVC asserts more like
other platforms, but it's quite cosmetic.
regards, tom lane
On Sun, Jul 30, 2023 at 9:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
John Naylor <john.naylor@enterprisedb.com> writes:
It seems that we should have "Assert(word != 0);" at the top, which
matches
the other platforms anyway, so I'll add that.
That's basically equivalent to the existing Assert(non_zero).
I think it'd be okay to drop that one and instead have
the same Assert condition as other platforms, but having both
would be redundant.
Works for me, so done that way for both forward and reverse variants. Since
the return value is no longer checked in any builds, I thought about
removing the variable containing it, but it seems best to leave it behind
for clarity since these are not our functions.
--
John Naylor
EDB: http://www.enterprisedb.com
John Naylor <john.naylor@enterprisedb.com> writes:
On Sun, Jul 30, 2023 at 9:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
That's basically equivalent to the existing Assert(non_zero).
I think it'd be okay to drop that one and instead have
the same Assert condition as other platforms, but having both
would be redundant.
Works for me, so done that way for both forward and reverse variants. Since
the return value is no longer checked in any builds, I thought about
removing the variable containing it, but it seems best to leave it behind
for clarity since these are not our functions.
Hmm, aren't you risking "variable is set but not used" warnings?
Personally I'd have made these like
(void) _BitScanReverse(&result, word);
Anybody trying to understand this code is going to have to look up
the man page for _BitScanReverse anyway, so I'm not sure that
keeping the variable adds much readability.
However, if no version of MSVC actually issues such a warning,
it's moot.
regards, tom lane
On Mon, Jul 31, 2023 at 5:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
John Naylor <john.naylor@enterprisedb.com> writes:
Works for me, so done that way for both forward and reverse variants.
Since
the return value is no longer checked in any builds, I thought about
removing the variable containing it, but it seems best to leave it
behind
for clarity since these are not our functions.
Hmm, aren't you risking "variable is set but not used" warnings?
Personally I'd have made these like(void) _BitScanReverse(&result, word);
I'd reasoned that such a warning would have showed up in non-assert builds
already, but I neglected to consider that those could have different
warning settings. For the time being drongo shows green, at least, but
we'll see.
--
John Naylor
EDB: http://www.enterprisedb.com