Re: Allowing printf("%m") only where it actually works

From: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Allowing printf("%m") only where it actually works
Date: 2018-05-27 05:04:50
Message-ID: CAEepm=1YLObd0aX9K4jqOHwQF3LQzSJB1VCNd-GstZhN67Q4CA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Sun, May 27, 2018 at 12:38 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> At least in the case of ereport, all it takes to create a hazard is
> more than one sub-function, eg this is risky:
>
> ereport(..., errmsg(..., strerror(errno)), errdetail(...));
>
> because errdetail() might run first and malloc some memory for its
> constructed string.
>
> So I think a blanket policy of "don't trust errno within the arguments"
> is a good idea, even though it might be safe to violate it in the
> existing cases in exec.c.

Right, malloc() is a hazard I didn't think about. I see that my local
malloc() makes an effort to save and restore errno around syscalls,
but even if all allocators were so thoughtful, which apparently they
aren't, there is also the problem that malloc itself can deliberately
set errno to ENOMEM per spec. I take your more general point that you
can't rely on anything we didn't write not trashing errno, even libc.

On Tue, May 22, 2018 at 4:03 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I noticed another can of worms here, too: on Windows, doesn't use of
> GetLastError() in elog/ereport have exactly the same hazard as errno?
> Or is there some reason to think it can't change value during errstart()?

Yeah, on Windows the same must apply, not in errstart() itself but any
time you pass more than one value to elog() using expressions that
call functions we can't audit for last-error-stomping.

Out of curiosity I tried adding a GetLastError variable for Windows
(to hide the function of that name and break callers) to the earlier
experimental patch (attached). I had to give it an initial value to
get rid of a warning about an unused variable (by my reading of the
documentation, __pragma(warning(suppress:4101)) can be used in macros
(unlike #pragma) and should shut that warning up, but it doesn't work
for me, not sure why). Of course that produces many errors since we
do that all over the place:

https://ci.appveyor.com/project/macdice/postgres/build/1.0.184

--
Thomas Munro
http://www.enterprisedb.com

Attachment Content-Type Size
prevent-errno-v2.patch application/octet-stream 2.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message MauMau 2018-05-27 05:20:01 I'd like to discuss scaleout at PGCon
Previous Message Tom Lane 2018-05-27 01:00:33 Re: Allowing printf("%m") only where it actually works