patch: prevent user from setting wal_buffers over 2GB bytes

Started by Josh Berkusover 10 years ago9 messages

josh@agliodbs.com

over 10 years ago

1 attachment(s)

Hackers,

In guc.c, the maximum for wal_buffers is INT_MAX. However, wal_buffers
is actually measured in 8KB buffers, not in bytes. This means that
users are able to set wal_buffers > 2GB. When the database is started,
this can cause a core dump if the WAL offset is > 2GB.

Attached patch fixes this issue by lowering the maximum for wal_buffers:

josh@Radegast:~/pg94a$ bin/pg_ctl -D data start
server starting
josh@Radegast:~/pg94a$ LOG: 393216 is outside the valid range for
parameter "wal_buffers" (-1 .. 262143)
FATAL: configuration file "/home/josh/pg94a/data/postgresql.conf"
contains errors

Thanks to Andrew Gierth for diagnosing this issue.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Attachments:

fix_wal_buffer_max.patchtext/x-patch; name=fix_wal_buffer_max.patchDownload

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
new file mode 100644
index 1b7b914..b3dac51
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** static struct config_int ConfigureNamesI
*** 2215,2221 ****
  			GUC_UNIT_XBLOCKS
  		},
  		&XLOGbuffers,
! 		-1, -1, INT_MAX,
  		check_wal_buffers, NULL, NULL
  	},
  
--- 2215,2221 ----
  			GUC_UNIT_XBLOCKS
  		},
  		&XLOGbuffers,
! 		-1, -1, (INT_MAX / XLOG_BLCKSZ),
  		check_wal_buffers, NULL, NULL
  	},

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Josh Berkus (#1)

Re: patch: prevent user from setting wal_buffers over 2GB bytes

On Thu, Jul 30, 2015 at 9:17 PM, Josh Berkus <josh@agliodbs.com> wrote:

In guc.c, the maximum for wal_buffers is INT_MAX. However, wal_buffers
is actually measured in 8KB buffers, not in bytes. This means that
users are able to set wal_buffers > 2GB. When the database is started,
this can cause a core dump if the WAL offset is > 2GB.

Why does this cause a core dump? We could consider fixing whatever
the problem is rather than capping the value.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Josh Berkus

josh@agliodbs.com

over 10 years ago

In reply to: Josh Berkus (#1)

Re: patch: prevent user from setting wal_buffers over 2GB bytes

On 07/31/2015 10:43 AM, Robert Haas wrote:

On Thu, Jul 30, 2015 at 9:17 PM, Josh Berkus <josh@agliodbs.com> wrote:

In guc.c, the maximum for wal_buffers is INT_MAX. However, wal_buffers
is actually measured in 8KB buffers, not in bytes. This means that
users are able to set wal_buffers > 2GB. When the database is started,
this can cause a core dump if the WAL offset is > 2GB.

Why does this cause a core dump? We could consider fixing whatever
the problem is rather than capping the value.

The underlying issue is that byte position in wal_buffers is a 32-bit
INT, so as soon as you exceed that, core dump.

Given that useful ranges for wal_buffers are in the 8MB to 128MB range,
I don't see that a cap at 2GB is much of a burden. For that reason, I'd
prefer capping the value to replumbing. We have not previously
documented the limit for this parameter, which is probably how the bug
happened in the first place. Clearly no user has been setting it above
2GB, or they would have been core dumping.

Oh, and yes, I think this should be backported; this issue exists in all
supported versions.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Import Notes

Reply to msg id not found: WM7752ccd5f87d5d7968f2183ebfd7462dff4498283359294ea4afcfb59e2ede9143113a12bccd18f6f024e26faff3f9bd@asav-2.01.com

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Josh Berkus (#3)

Re: patch: prevent user from setting wal_buffers over 2GB bytes

On Fri, Jul 31, 2015 at 8:09 PM, Josh Berkus <josh@agliodbs.com> wrote:

On 07/31/2015 10:43 AM, Robert Haas wrote:

On Thu, Jul 30, 2015 at 9:17 PM, Josh Berkus <josh@agliodbs.com> wrote:

In guc.c, the maximum for wal_buffers is INT_MAX. However, wal_buffers
is actually measured in 8KB buffers, not in bytes. This means that
users are able to set wal_buffers > 2GB. When the database is started,
this can cause a core dump if the WAL offset is > 2GB.

Why does this cause a core dump? We could consider fixing whatever
the problem is rather than capping the value.

The underlying issue is that byte position in wal_buffers is a 32-bit
INT, so as soon as you exceed that, core dump.

OK. So capping it sounds like the right approach, then.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Takashi Horikawa

t-horikawa@aj.jp.nec.com

over 10 years ago

In reply to: Robert Haas (#4)

2 attachment(s)

Re: patch: prevent user from setting wal_buffers over 2GB bytes

Why does this cause a core dump? We could consider fixing whatever
the problem is rather than capping the value.

As far as I experiment with my own evaluation environment using
PostgreSQL-9.4.4 on a x86_64 Linux, this problem can be fixed with the patch
attached.

I have confirmed that applying the patch makes 'wal_buffers = 4GB' works
fine, while original PostgreSQL-9.4.4 results in core dump (segfault). I'll be
happy if anyone reconfirm this.

--
Takashi Horikawa
NEC Corporation
Knowledge Discovery Research Laboratories

Show quoted text

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas
Sent: Tuesday, August 04, 2015 2:29 AM
To: Josh Berkus
Cc: PostgreSQL-development
Subject: Re: [HACKERS] patch: prevent user from setting wal_buffers over
2GB bytes

On Fri, Jul 31, 2015 at 8:09 PM, Josh Berkus <josh@agliodbs.com> wrote:

On 07/31/2015 10:43 AM, Robert Haas wrote:

On Thu, Jul 30, 2015 at 9:17 PM, Josh Berkus <josh@agliodbs.com> wrote:

In guc.c, the maximum for wal_buffers is INT_MAX. However,
wal_buffers is actually measured in 8KB buffers, not in bytes. This
means that users are able to set wal_buffers > 2GB. When the
database is started, this can cause a core dump if the WAL offset is

2GB.

Why does this cause a core dump? We could consider fixing whatever
the problem is rather than capping the value.

The underlying issue is that byte position in wal_buffers is a 32-bit
INT, so as soon as you exceed that, core dump.

OK. So capping it sounds like the right approach, then.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL
Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachments:

allow_wal_buffer_over_2G.patchapplication/octet-stream; name=allow_wal_buffer_over_2G.patchDownload

*** postgresql-9.4.4/src/backend/access/transam/xlog.c	2015-06-10 04:29:38.000000000 +0900
--- postgresql-9.4.4-xlog/src/backend/access/transam/xlog.c	2015-08-04 12:05:02.418718384 +0900
***************
*** 7176,7182 ****
  		firstIdx = XLogRecPtrToBufIdx(EndOfLog);
  
  		/* Copy the valid part of the last block, and zero the rest */
! 		page = &XLogCtl->pages[firstIdx * XLOG_BLCKSZ];
  		len = EndOfLog % XLOG_BLCKSZ;
  		memcpy(page, xlogreader->readBuf, len);
  		memset(page + len, 0, XLOG_BLCKSZ - len);
--- 7176,7182 ----
  		firstIdx = XLogRecPtrToBufIdx(EndOfLog);
  
  		/* Copy the valid part of the last block, and zero the rest */
! 		page = &XLogCtl->pages[firstIdx * (Size) XLOG_BLCKSZ];
  		len = EndOfLog % XLOG_BLCKSZ;
  		memcpy(page, xlogreader->readBuf, len);
  		memset(page + len, 0, XLOG_BLCKSZ - len);

smime.p7sapplication/pkcs7-signature; name=smime.p7sDownload

Tom Lane

tgl@sss.pgh.pa.us

over 10 years ago

In reply to: Takashi Horikawa (#5)

Re: patch: prevent user from setting wal_buffers over 2GB bytes

Takashi Horikawa <t-horikawa@aj.jp.nec.com> writes:

Why does this cause a core dump? We could consider fixing whatever
the problem is rather than capping the value.

As far as I experiment with my own evaluation environment using
PostgreSQL-9.4.4 on a x86_64 Linux, this problem can be fixed with the patch
attached.

I'm unsure whether this represents a complete fix ... but even if it does,
it would be awfully easy to re-introduce similar bugs in future code
changes, and who would notice? Josh's approach of restricting the buffer
size seems a lot more robust.

If there were any practical use-case for such large WAL buffers then it
might be worth spending some effort/risk here. But AFAICS, there is not.
Indeed, capping wal_buffers might be argued to be a good thing in itself
because it would prevent users from wasting shared memory foolishly.

So my vote is for the original approach. (I've not read Josh's patch,
so there might be something wrong with it in detail, but I like the
basic approach.)

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund

andres@anarazel.de

over 10 years ago

In reply to: Tom Lane (#6)

Re: patch: prevent user from setting wal_buffers over 2GB bytes

On 2015-08-04 09:49:58 -0400, Tom Lane wrote:

Takashi Horikawa <t-horikawa@aj.jp.nec.com> writes:

Why does this cause a core dump? We could consider fixing whatever
the problem is rather than capping the value.

As far as I experiment with my own evaluation environment using
PostgreSQL-9.4.4 on a x86_64 Linux, this problem can be fixed with the patch
attached.

I'm unsure whether this represents a complete fix ... but even if it does,
it would be awfully easy to re-introduce similar bugs in future code
changes, and who would notice? Josh's approach of restricting the buffer
size seems a lot more robust.

If there were any practical use-case for such large WAL buffers then it
might be worth spending some effort/risk here. But AFAICS, there is not.
Indeed, capping wal_buffers might be argued to be a good thing in itself
because it would prevent users from wasting shared memory foolishly.

So my vote is for the original approach. (I've not read Josh's patch,
so there might be something wrong with it in detail, but I like the
basic approach.)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas

robertmhaas@gmail.com

over 10 years ago

In reply to: Andres Freund (#7)

Re: patch: prevent user from setting wal_buffers over 2GB bytes

On Tue, Aug 4, 2015 at 9:52 AM, Andres Freund <andres@anarazel.de> wrote:

So my vote is for the original approach. (I've not read Josh's patch,
so there might be something wrong with it in detail, but I like the
basic approach.)

+1

OK, committed and back-patched that all the way back to 9.0.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Takashi Horikawa

t-horikawa@aj.jp.nec.com

over 10 years ago

In reply to: Tom Lane (#6)

1 attachment(s)

Re: patch: prevent user from setting wal_buffers over 2GB bytes

... Josh's approach of restricting the buffer size seems
a lot more robust.

I understand that the capping of approach of restricting the buffer size
is much more robust and is suitable in this case.

I, howerver, think that the chane from
'page = &XLogCtl->pages[firstIdx * XLOG_BLCKSZ];'
to
'page = &XLogCtl->pages[firstIdx * (Size) XLOG_BLCKSZ];'
is no harm even when restricting the wal buffer size.

It is in harmony with the usage of 'XLogCtl->pages' found in, for example,
'cachedPos = XLogCtl->pages + idx * (Size) XLOG_BLCKSZ;'
in GetXLogBuffer(XLogRecPtr ptr)
and
'NewPage = (XLogPageHeader) (XLogCtl->pages + nextidx * (Size) XLOG_BLCKSZ);
'
in AdvanceXLInsertBuffer(XLogRecPtr upto, bool opportunistic)
, etc.

Only exception is
'page = &XLogCtl->pages[firstIdx * XLOG_BLCKSZ];'
in
StartupXLOG(void)
--
Takashi Horikawa
NEC Corporation
Knowledge Discovery Research Laboratories

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org
[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Tom Lane
Sent: Tuesday, August 04, 2015 10:50 PM
To: Horikawa Takashi(堀川隆)
Cc: PostgreSQL-development
Subject: Re: [HACKERS] patch: prevent user from setting wal_buffers over
2GB bytes

Takashi Horikawa <t-horikawa@aj.jp.nec.com> writes:

Why does this cause a core dump? We could consider fixing whatever
the problem is rather than capping the value.

As far as I experiment with my own evaluation environment using
PostgreSQL-9.4.4 on a x86_64 Linux, this problem can be fixed with the
patch attached.

I'm unsure whether this represents a complete fix ... but even if it does,
it would be awfully easy to re-introduce similar bugs in future code

changes,

and who would notice? Josh's approach of restricting the buffer size

seems

Show quoted text

a lot more robust.

If there were any practical use-case for such large WAL buffers then it
might be worth spending some effort/risk here. But AFAICS, there is not.
Indeed, capping wal_buffers might be argued to be a good thing in itself
because it would prevent users from wasting shared memory foolishly.

So my vote is for the original approach. (I've not read Josh's patch, so
there might be something wrong with it in detail, but I like the basic
approach.)

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make
changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers