On Sun, May 27, 2018 at 12:38 PM, Tom Lane <t...@sss.pgh.pa.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 <t...@sss.pgh.pa.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
prevent-errno-v2.patch
Description: Binary data
