Date: Thu, 9 Jan 2020 14:45:45 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andrey Borodin <x4mmm@yandex-team.ru>,
	Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	Adam Scott <adam.c.scott@gmail.com>,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16122: segfault pg_detoast_datum (datum=0x0) at fmgr.c:1833
 numrange query
Message-ID: <20200109054545.GL2251@paquier.xyz>
References: 
 <CA+s62-Mnj-x8_ZXh7tBVU3WJ3cKUsUL2HAp2YaJSXQPw1iypFw@mail.gmail.com>
 <20191119014827.GC1614@paquier.xyz>
 <20191119103813.xnsloisoxkwrxoim@development>
 <20191119114056.GA516103@paquier.xyz>
 <20191210053815.GI72921@paquier.xyz>
 <837E8CEE-FFF3-497C-B7C2-37BB42FAF2C5@yandex-team.ru>
 <DE43C854-F3BD-4247-93F3-2774B643445C@yandex-team.ru>
 <5794.1578342514@sss.pgh.pa.us>
 <20200107063907.GF2386@paquier.xyz>
 <23459.1578409687@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="cs5saTBZh7UZl2eX"
Content-Disposition: inline
In-Reply-To: <23459.1578409687@sss.pgh.pa.us>
Precedence: bulk


--cs5saTBZh7UZl2eX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Jan 07, 2020 at 10:08:07AM -0500, Tom Lane wrote:
> Hmm ... maybe, but I'm not entirely convinced that they should be
> mirror images.  The endpoint handling is the same right now, but
> should it be?  Anyway, merging them seems out of scope for a bug fix.

Sure.  That was just a random though while reading again the code.

> I wouldn't mind having a test case if it crashes reliably, but it
> doesn't for me.  I poked into why not, and realized that even if
> get_position is passed a pointer to garbage data in hist2, it will
> not crash unless the "!hist2->infinite" test succeeds, because
> otherwise it won't call the rng_subdiff function.  Depending
> on compiler and optimization level, that usually means that any
> nonzero value in that byte will prevent a crash.  Given that the
> whole point here is we're accessing memory that hasn't been
> initialized by this code, we don't have any way to make it likely
> that that won't be true.

Ah, OK.  That makes sense.  If you don't wish to keep the test around
in core, that's fine to me.  I may keep that into one of my own repos
just for history's sake.

> BTW, I noticed that at -O0, my compiler generates a code sequence
> that effectively implements that test as "hist2->infinite !=3D true",
> ie it will succeed for any bit pattern that's not exactly 0x01.
> This greatly raises the odds of seeing the crash of course (so maybe
> your compiler is doing likewise?).  I find that rather disturbing
> because my mental model has always been that any nonzero bit pattern
> in a bool would be interpreted as true.  Apparently the gcc boys think
> that they're entitled to consider _Bool values other than exactly 0
> and 1 as undefined, which is scary because I'm not sure we are totally
> consistent about that everywhere.  It definitely makes it harder to
> reason about what will happen with garbage memory contents.  (This
> might also explain why you saw a change in behavior at 9a95a77d9d.)

That would not be the first only time my environment behaves different
than the others.  My build scripts have been pretty good at catching
issues with compiled code, particularly when it comes to ordering.

>> -   if (bin_width <=3D 0.0)
>> +   if (isnan(bin_width) || bin_width <=3D 0.0)
>>  	return 0.5;			/* zero width bin */
>> This comment is incorrect now?
>=20
> Yeah, it could be improved.  Maybe "punt for NaN or zero-width bin"?

Sounds fine to me.

>> +   if (isnan(position))
>> +       return 0.5;			/* punt if we have any NaNs or Infs */
>> +
>> This comment is incorrect as well, no?  In this case both histograms
>> have finite bounds, and you just check if that's a number.
>=20
> The point is that we could get a NaN from the division either from
> the rng_subdiff function returning NaN, or from an undefined case
> such as Inf/Inf.  Maybe write "punt for NaN input, Inf/Inf, etc"?
> (We don't need to test for Inf explicitly, because the tests just
> below can deal with that.)

I would rewrite this comment for clarity.  But I am fine to leave that
to you at the end if you feel differently.
--
Michael

--cs5saTBZh7UZl2eX
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl4WvgkACgkQnvQgOdby
QH1INg/7BshF0rboSttofvM5peXhY9n7E5WuBN1gL1jDh/dP0DpgkIpL8YVgG6Xm
C33mIcePJNOG/IiUe65Kkca9+ECaiwn8BGGmtW5auR2Mvm6E3URe7btMc9ITSbL1
LCUMyBma5h5avsVCW6At2LbIvnqy2mhJBxnrc15J8hIE2WY2kVKEJiojQlrWwF9s
n1v8ZZ/dd8NVwU46Tkjn0RcvKbf8lhJ5ri9bnLb+SNmdXWv4vjj1L40vIxqof3U+
43Fth8A0D/0PcBD833blZO8DOw72CsSsyYRalAcrov9Jh3eqg6rkNOHk2K8WiclA
uaKyPHYG8TSfro4lPIh39w2Axlk49nbQEV9YIIqSpSyzX2Kq8JtAu5piikAlMrEX
qdWzcYBT818SjIl0mhUpZ7xGvvoT6ptGXU3JX5DRrcKp9jkMwS3ihG9fvRyQrAfq
iVngIvZqcAHxlrZXp8oPVsVCDG8J7ftKqhwjyuZh47ZMadkDbXZakZTgxmB+Agax
4FFmLacDupP7K+OhS2JP6b3ua7qZ2+3pE82cYEBnd8/krAUZiVtV+ri8NKd36zyR
QayoXVlphpBzDRmqiznXMp2MTG4THi9d1wwpFO1bBAip4ADEKMO1krUwydzudH1m
T8FnGPzlr329Bw2bjPD1pEM/Zj9Gp38Qm5GNqyOKXu3pIpTkMkg=
=H3I6
-----END PGP SIGNATURE-----

--cs5saTBZh7UZl2eX--