Thread: [PATCH] Add error handling to byteaout.
Hi, when dealing with bytea values that are below the 1GB limit, but too large to be escaped, the error messages emitted are not very helpful for users. Especially if they appear in an unrelated context such as during pg_dump. I've attached a patch that adds ereport()ing that would have prevented some confusion. Example: ,----[ master ] | ase=# select mkbytea(29); | ERROR: invalid memory alloc request size 1073741827 | ase=# set bytea_output to escape; | ase=# select mkbytea(29); | ERROR: invalid memory alloc request size 18446744071562067969 `---- The scary one is due to an integer overflow the attached patch also fixes. I don't see any security implications though as it's only the sign bit that is affected. ,----[ with patch applied ] | ase=# set bytea_output to 'escape'; | ase=# select mkbytea(29); | ERROR: escaped bytea value would be too big | DETAIL: Value would require 2147483649 bytes. | HINT: Use a different bytea_output setting or binary methods such as COPY BINARY. `---- regards, Andreas create function mkbytea(power int, part bytea default '\x00') returns bytea as $$select case when power>0 thenmkbytea(power-1,part||part) else part end;$$ language sql;
Andreas Seltenreich <andreas.seltenreich@credativ.de> writes:
> The scary one is due to an integer overflow the attached patch also
> fixes.
s/int/Size/ doesn't fix anything on 32-bit machines.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Andreas Seltenreich <andreas.seltenreich@credativ.de> writes: >> The scary one is due to an integer overflow the attached patch also >> fixes. > > s/int/Size/ doesn't fix anything on 32-bit machines. Well, it changes the signedness of the computation on 32-bit, and in combination with the fact that "len" is always smaller than 2^32, but may exceed 2^31-1, the change avoids the dependency on the undefined behavior of signed integer overflows in C on 32-bit as well. But I admit that this might be a rather academic point... Anyway, my motivation for the patch was the improved error reporting. Is the drive-by type change a problem here? regards, Andreas
On 06/02/2015 06:47 PM, Andreas Seltenreich wrote: > Tom Lane <tgl@sss.pgh.pa.us> writes: > >> Andreas Seltenreich <andreas.seltenreich@credativ.de> writes: >>> The scary one is due to an integer overflow the attached patch also >>> fixes. >> >> s/int/Size/ doesn't fix anything on 32-bit machines. > > Well, it changes the signedness of the computation on 32-bit, and in > combination with the fact that "len" is always smaller than 2^32, but > may exceed 2^31-1, the change avoids the dependency on the undefined > behavior of signed integer overflows in C on 32-bit as well. But I > admit that this might be a rather academic point... > Postgres requires twos-complement representation, so that the assumption that signed integer types wrap around on overflow can be safely made.
Andreas Seltenreich wrote: > Tom Lane <tgl@sss.pgh.pa.us> writes: > > > Andreas Seltenreich <andreas.seltenreich@credativ.de> writes: > >> The scary one is due to an integer overflow the attached patch also > >> fixes. > > > > s/int/Size/ doesn't fix anything on 32-bit machines. > > Well, it changes the signedness of the computation on 32-bit, and in > combination with the fact that "len" is always smaller than 2^32, but > may exceed 2^31-1, the change avoids the dependency on the undefined > behavior of signed integer overflows in C on 32-bit as well. Why not just use an unsigned 64 bit variable? Also, perhaps palloc_huge() avoids the whole problem in the first place ... though it might only move the issue around, if you cannot ship the longer-than-1GB resulting escaped value. (Of course, if you try to allocate 2 GB in a 32 bit machine, you're going to be having quite some fun ...) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera writes:
> Why not just use an unsigned 64 bit variable? Also, perhaps
> palloc_huge() avoids the whole problem in the first place ...
Ja, that crossed my mind too, but the current limit is already far
beyond anything that is usually configured for per-backend memory use,
so I dismissed it.
> though it might only move the issue around, if you cannot ship the
> longer-than-1GB resulting escaped value.
For example, when client and server encodings do not match:
,----[ mbutils.c ]
| result = palloc(len * MAX_CONVERSION_GROWTH + 1);
`----
This results in the fun fact that the maximum size for bytea values that
are guaranteed to be pg_dumpable regardless of encoding/escaping
settings is lower than 64MB.
One thing that would also mitigate the problem is supporting a more
efficient output format. For example, there's already means for
base64-encoding in the backend:
self=# select c, length(encode(mkbytea(28),c)) from (values ('hex'),('base64')) as v(c); c | length
--------+-----------hex | 536870912base64 | 362623337
(2 rows)
Maybe it is reasonable to make it available as another option for use in
bytea_output?
regards,
Andreas
Piotr Stefaniak writes: >>> s/int/Size/ doesn't fix anything on 32-bit machines. > > Postgres requires twos-complement representation, so that the > assumption that signed integer types wrap around on overflow can be > safely made. Thanks for the clarification!
On June 3, 2015 9:42:21 PM GMT+02:00, Andreas Seltenreich <andreas.seltenreich@credativ.de> wrote: >Piotr Stefaniak writes: >>>> s/int/Size/ doesn't fix anything on 32-bit machines. >> >> Postgres requires twos-complement representation, so that the >> assumption that signed integer types wrap around on overflow can be >> safely made. Uh, no. The C standard defines signed overflow as undefined, an compilers assume it doesn't happen. The optimize based ony observation. --- Please excuse brevity and formatting - I am writing this on my mobile phone.
<div dir="ltr"><br /><div class="gmail_extra"><br /><div class="gmail_quote">On Thu, Jun 4, 2015 at 1:32 AM, Alvaro Herrera<span dir="ltr"><<a href="mailto:alvherre@2ndquadrant.com" target="_blank">alvherre@2ndquadrant.com</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px#ccc solid;padding-left:1ex"><span class="">Andreas Seltenreich wrote:<br /> > Tom Lane <<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>writes:<br /> ><br /> > > Andreas Seltenreich <<a href="mailto:andreas.seltenreich@credativ.de">andreas.seltenreich@credativ.de</a>>writes:<br /> > >> The scaryone is due to an integer overflow the attached patch also<br /> > >> fixes.<br /> > ><br /> > >s/int/Size/ doesn't fix anything on 32-bit machines.<br /> ><br /> > Well, it changes the signedness of the computationon 32-bit, and in<br /> > combination with the fact that "len" is always smaller than 2^32, but<br /> >may exceed 2^31-1, the change avoids the dependency on the undefined<br /> > behavior of signed integer overflowsin C on 32-bit as well.<br /><br /></span>Why not just use an unsigned 64 bit variable? Also, perhaps<br /> palloc_huge()avoids the whole problem in the first place ... though it<br /> might only move the issue around, if you cannotship the longer-than-1GB<br /> resulting escaped value. (Of course, if you try to allocate 2 GB in a<br /> 32 bitmachine, you're going to be having quite some fun ...)<span class="HOEnZb"></span><br /></blockquote></div><br /></div><divclass="gmail_extra">Pure nitpicking: there is no palloc_huge, only repalloc_huge. Though we could have one.<br/>-- <br /><div class="gmail_signature">Michael<br /></div></div></div>