From: | "movead(dot)li(at)highgo(dot)ca" <movead(dot)li(at)highgo(dot)ca> |
---|---|
To: | "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | ashutosh(dot)bapat <ashutosh(dot)bapat(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: A bug when use get_bit() function for a long bytea string |
Date: | 2020-04-02 06:26:08 |
Message-ID: | 2020040214260375403022@highgo.ca |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
>I don't think this has really solved the overflow hazards. For example,
>in binary_encode we've got
>resultlen = enc->encode_len(VARDATA_ANY(data), datalen);
>result = palloc(VARHDRSZ + resultlen);
>and all you've done about that is changed resultlen from int to int64.
>On a 64-bit machine, sure, palloc will be able to detect if the
>result exceeds what can be allocated --- but on a 32-bit machine
>it'd be possible for the size_t argument to overflow altogether.
>(Or if you want to argue that it can't overflow because no encoder
>expands the data more than 4X, then we don't need to be making this
>change at all.)
>I don't think there's really any way to do that safely without an
>explicit check before we call palloc.
I am sorry I do not very understand these words, and especially
what's the mean by 'size_t'.
Here I change resultlen from int to int64, is because we can get a right
error report value other than '-1' or another strange number.
>Why did you change the outputs from unsigned to signed? Why didn't
>you change the dlen inputs? I grant that we know that the input
>can't exceed 1GB in Postgres' usage, but these signatures are just
>randomly inconsistent, and you didn't even add a comment explaining
>why. Personally I think I'd make them like
>uint64 (*encode_len) (const char *data, size_t dlen);
>which makes it explicit that the dlen argument describes the length
>of a chunk of allocated memory, while the result might exceed that.
I think it makes sence and followed.
>Lastly, there is a component of this that can be back-patched and
>a component that can't --- we do not change system catalog contents
>in released branches. Cramming both parts into the same patch
>is forcing the committer to pull them apart, which is kind of
>unfriendly.
Sorry about that, attached is the changed patch for PG13, and the one
for older branches will send sooner.
Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca
Attachment | Content-Type | Size |
---|---|---|
long_bytea_string_bug_fix_ver6.patch | application/octet-stream | 10.9 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Julien Rouhaud | 2020-04-02 06:29:31 | Re: User Interface for WAL usage data |
Previous Message | Michael Paquier | 2020-04-02 06:21:50 | Re: potential stuck lock in SaveSlotToPath() |