Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix out of range #82

Closed
wants to merge 1 commit into from
Closed

Fix out of range #82

wants to merge 1 commit into from

Conversation

hidva
Copy link
Contributor

@hidva hidva commented Sep 26, 2019

Before fix in multiset_unpack():
image
After fix:
image
The hlltest.sql is here

Before fix in check_modifiers():
image
After fix:
image

$pg_config 
BINDIR = /home/pg/pgbin/bin
DOCDIR = /home/pg/pgbin/share/doc/postgresql
HTMLDIR = /home/pg/pgbin/share/doc/postgresql
INCLUDEDIR = /home/pg/pgbin/include
PKGINCLUDEDIR = /home/pg/pgbin/include/postgresql
INCLUDEDIR-SERVER = /home/pg/pgbin/include/postgresql/server
LIBDIR = /home/pg/pgbin/lib
PKGLIBDIR = /home/pg/pgbin/lib/postgresql
LOCALEDIR = /home/pg/pgbin/share/locale
MANDIR = /home/pg/pgbin/share/man
SHAREDIR = /home/pg/pgbin/share/postgresql
SYSCONFDIR = /home/pg/pgbin/etc/postgresql
PGXS = /home/pg/pgbin/lib/postgresql/pgxs/src/makefiles/pgxs.mk
CONFIGURE = '--enable-cassert' '--prefix=/home/pg/pgbin' '--enable-debug' 'CFLAGS=-O0 -fsanitize=address' 'LDFLAGS=-fsanitize=address'
CC = gcc
CPPFLAGS = -D_GNU_SOURCE
CFLAGS = -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O0 -fsanitize=address
CFLAGS_SL = -fPIC
LDFLAGS = -fsanitize=address -Wl,--as-needed -Wl,-rpath,'/home/pg/pgbin/lib',--enable-new-dtags
LDFLAGS_EX = 
LDFLAGS_SL = 
LIBS = -lpgcommon -lpgport -lz -lreadline -lrt -lcrypt -ldl -lm 
VERSION = PostgreSQL 9.6.15

@citus-bot
Copy link

Hi @hidva, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement.

@hidva hidva changed the title Fix stackoverflow in compressed_add Fix out of range Sep 27, 2019
@hidva hidva force-pushed the patch-1 branch 3 times, most recently from 164139f to 2f1df76 Compare September 27, 2019 04:20
@hidva
Copy link
Contributor Author

hidva commented Sep 27, 2019

I cannot receive verify letter from CITUSDATA.

@@ -1388,7 +1416,7 @@ multiset_unpack(multiset_t * o_msp,
}

// Make sure the explicit array fits in memory.
if ((i_size - hdrsz) > MS_MAXDATA)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not forget 8 bytes occupy by ms_explicit_t::mse_nelem!

So this should be (i_size - hdrsz) > (MS_MAXDATA - sizeof(size_t))!

@hidva
Copy link
Contributor Author

hidva commented Sep 27, 2019

Fix fail in make installcheck later~

@hidva hidva force-pushed the patch-1 branch 4 times, most recently from f1c2056 to c8c16b7 Compare September 27, 2019 09:27
@citus-bot
Copy link

You did it @hidva!

Thank you for signing the Citus Data Contributor License Agreement. We can now accept this contribution and all future contributions from you.

Somebody from our team will proceed with code review.

@serprex
Copy link
Collaborator

serprex commented Nov 7, 2019

All looks fine. Bit unnerving having regression output be architecture dependent

* But we couldn't hard code it explicitly because we do not know current alignment schema
* when compiling it.
*/
static size_t
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions should have prototypes declared at top of file with other static prototypes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems there are no declarations for most other functions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants