Обсуждение: Data race in interfaces/libpq/fe-exec.c
This line https://github.com/postgres/postgres/blob/30012a04a6c8127397a8ab71e160d9c7e7fbe874/src/interfaces/libpq/fe-exec.c#L1073
triggers data race errors when run under ThreadSanitizer (*)
As far as I can tell, the static variable in question is a hack to allow a couple of deprecated functions that are already unsafe to use (PQescapeString and PQescapeBytea) to be fractionally less unsafe to use.
Would there be any interest in a patch changing the type of static_client_coding and static_std_strings to some atomic equivalent, so the data race goes away?
I know that as long as clients aren't daft enough to call PQescapeString or PQescapeBytea, then the static variables are write-only - but wiser heads than mine assure me that even if nothing ever reads from the variables, unguarded simultaneous writes can cause issues when built with particularly aggressive compilers... On the plus side: "As a note for changing the variable to be atomic - if it uses acquire-release semantics, it'll be free on x86 (barring any potential compiler optimisations that we want to stop). And relaxed semantics should be free everywhere."
And it would be nice to be able to keep running my postgres-using tests under ThreadSanitizer...
thanks
Mark
(*) specifically in a test that kicks off two simultaneous threads whose first action is to open a postgres connection.
Mark Charsley <mcharsley@google.com> writes: > This line > https://github.com/postgres/postgres/blob/30012a04a6c8127397a8ab71e160d9c7e7fbe874/src/interfaces/libpq/fe-exec.c#L1073 > triggers data race errors when run under ThreadSanitizer (*) > As far as I can tell, the static variable in question is a hack to allow a > couple of deprecated functions that are already unsafe to use > (PQescapeString and PQescapeBytea) to be fractionally less unsafe to use. Yup. > Would there be any interest in a patch changing the type of > static_client_coding > and static_std_strings > <https://github.com/postgres/postgres/blob/30012a04a6c8127397a8ab71e160d9c7e7fbe874/src/interfaces/libpq/fe-exec.c#L49> > to > some atomic equivalent, so the data race goes away? I don't see that making those be some other datatype would improve anything usefully. (1) On just about every platform known to man, int and bool are going to be atomic anyway. (2) The *actual* hazards here, as opposed to theoretical ones, are that you're using more than one connection with different settings for these values, whereupon it's not clear whether those deprecated functions will see the appropriate settings when they're used. A different data type won't help that. In short: this warning you're getting from ThreadSanitizer is entirely off-point, so contorting the code to suppress it seems useless. regards, tom lane
According to folks significantly cleverer than me, this can be a problem: See section 2.4 in https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf
tl;dr in a self-fulfilling prophecy kind of way, there are no benign data-races. So the compiler can assume no-one would write a data race. Therefore it can make aggressive optimisations that render what would otherwise have been a benign race actively dangerous.
Granted the danger here is mainly theoretical, and the main problem for me is that turning off ThreadSanitizer because of this issue means that other more dangerous issues in my code (rather than the postgres client code) won't be found. But the above is the reason why ThreadSanitizer folks don't want to put in any "you can ignore this race, it's benign" functionality, and told me that the right thing to do was to contact you folks and get a fix in upstream...
Mark
On Thu, Jan 30, 2020 at 4:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Mark Charsley <mcharsley@google.com> writes:
> This line
> https://github.com/postgres/postgres/blob/30012a04a6c8127397a8ab71e160d9c7e7fbe874/src/interfaces/libpq/fe-exec.c#L1073
> triggers data race errors when run under ThreadSanitizer (*)
> As far as I can tell, the static variable in question is a hack to allow a
> couple of deprecated functions that are already unsafe to use
> (PQescapeString and PQescapeBytea) to be fractionally less unsafe to use.
Yup.
> Would there be any interest in a patch changing the type of
> static_client_coding
> and static_std_strings
> <https://github.com/postgres/postgres/blob/30012a04a6c8127397a8ab71e160d9c7e7fbe874/src/interfaces/libpq/fe-exec.c#L49>
> to
> some atomic equivalent, so the data race goes away?
I don't see that making those be some other datatype would improve anything
usefully. (1) On just about every platform known to man, int and bool are
going to be atomic anyway. (2) The *actual* hazards here, as opposed to
theoretical ones, are that you're using more than one connection with
different settings for these values, whereupon it's not clear whether
those deprecated functions will see the appropriate settings when they're
used. A different data type won't help that.
In short: this warning you're getting from ThreadSanitizer is entirely
off-point, so contorting the code to suppress it seems useless.
regards, tom lane