Thread: Small fix: avoid passing null pointers to memcpy()
Hello, from running the regression test suite (including TAP tests) and also sqlsmith, I've got a couple of places where UBSan reported calls to memcpy() with null pointer passed as either source or destination. Patch attached.
Attachment
On 2016-04-03 09:24, Piotr Stefaniak wrote: > from running the regression test suite (including TAP tests) and also > sqlsmith, I've got a couple of places where UBSan reported calls to > memcpy() with null pointer passed as either source or destination. > > Patch attached. Patch updated. Since this time the patch includes fixes for other standard library function calls (memset and bsearch), I'm renaming the patch file to be more generic.
Attachment
Added a fix for src/backend/storage/ipc/shm_mq.c:shm_mq_receive.
Attachment
On 4/16/16 8:48 AM, Piotr Stefaniak wrote: > On 2016-04-03 09:24, Piotr Stefaniak wrote: >> > from running the regression test suite (including TAP tests) and also >> > sqlsmith, I've got a couple of places where UBSan reported calls to >> > memcpy() with null pointer passed as either source or destination. >> > >> > Patch attached. > Patch updated. > > Since this time the patch includes fixes for other standard library > function calls (memset and bsearch), I'm renaming the patch file to be > more generic. Most of these changes appear to address the case where the size argument of memcpy() is zero. I'm not sure why those changes are necessary. At least in some cases that I sporadically checked, a zero size does not lead to a null pointer argument. We'll need some more details here. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi A smaller version removing memset in print_aligned_text function. The line is redundant , header_done isn't used yet and it's either pg_malloc0 or null. Without this patch make check fails 3 tests if pg is compiled with -fsanitize=address,undefined
Attachment
didier <did447@gmail.com> writes:
> A smaller version removing memset in print_aligned_text function.
> The line is redundant , header_done isn't used yet and it's either
> pg_malloc0 or null.
Hm, I see the theoretical problem ...
> Without this patch make check fails 3 tests if pg is compiled with
> -fsanitize=address,undefined
... but if that's the only evidence of an actual problem, I can't
get excited about it. ASAN complains about many things in Postgres,
and most of them are pretty hypothetical.
regards, tom lane
Hi, On Fri, May 24, 2019 at 5:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > didier <did447@gmail.com> writes: > > A smaller version removing memset in print_aligned_text function. > > The line is redundant , header_done isn't used yet and it's either > > pg_malloc0 or null. > > Hm, I see the theoretical problem ... > > > Without this patch make check fails 3 tests if pg is compiled with > > -fsanitize=address,undefined > > ... but if that's the only evidence of an actual problem, I can't > get excited about it. ASAN complains about many things in Postgres, Not that much, make check has only this one. > and most of them are pretty hypothetical. Well there's no point for this line even without ASAN, why call memset on header_done but not on width_header, width_average, and so on? ASAN complaining at runtime because memset is called with a null ptr and a zero count is just the nudge for removing it. Regards Didier
I wrote:
> didier <did447@gmail.com> writes:
>> Without this patch make check fails 3 tests if pg is compiled with
>> -fsanitize=address,undefined
> ... but if that's the only evidence of an actual problem, I can't
> get excited about it. ASAN complains about many things in Postgres,
> and most of them are pretty hypothetical.
BTW, I did try the described test case, so I suppose I might as well
summarize the results while I have them. The postmaster log from
a regression run with today's HEAD shows ASAN errors at
clog.c:299:3:
indexcmds.c:1054:4:
relcache.c:5905:6:
snapmgr.c:597:2:
snapmgr.c:601:2:
xact.c:5204:3:
The above all seem to be the same ilk as the problem in print.c,
ie passing a NULL pointer with zero count to memcpy, memset, or
the like. At least some of them are fairly old.
pg_crc32c_sse42.c:37:18:
pg_crc32c_sse42.c:44:9:
This is an intentional use of unaligned access:
* NB: We do unaligned accesses here. The Intel architecture allows that,
* and performance testing didn't show any performance gain from aligning
* the begin address.
This comment is unclear about whether it would actually hurt to have a
preparatory loop to align the begin address.
... and a whole bunch of places in arrayaccess.h and arrayfuncs.c.
These seem to be down to use of AnyArrayType:
typedef union AnyArrayType
{
ArrayType flt;
ExpandedArrayHeader xpn;
} AnyArrayType;
ASAN seems to believe that use of this union entitles the compiler to
assume 8-byte alignment even when touching fields of a short-header
array. Maybe it's right, but this code has been like this for awhile
and we've not heard trouble reports. I'm afraid that avoiding the
use of the union would be a notational mess, so I'm loath to change
it unless we're forced to.
This is just from the core tests, there could well be more issues in
contrib or PLs.
regards, tom lane
Hi,
On Fri, May 24, 2019 at 6:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> These seem to be down to use of AnyArrayType:
>
> typedef union AnyArrayType
> {
> ArrayType flt;
> ExpandedArrayHeader xpn;
> } AnyArrayType;
>
> ASAN seems to believe that use of this union entitles the compiler to
> assume 8-byte alignment even when touching fields of a short-header
In my understanding union has to be aligned to ExpandedArrayHeader (8
bytes) or it's a UB.
On x64 it could be an issue if AnyArrayType alignment is less than 4,
sse is enable and suddenly compiler choose to use sse instructions
with 16 bytes requirement then compiler may not emit the right code.
It's not rhetorical, big surprise first time you get a SIGBUS signal,
or a SIGFPE doing integer math, on x64.
Regards
Didier