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: Avoid integer overflow for large arguments #946

Merged
merged 2 commits into from Oct 24, 2017

Conversation

Projects
None yet
6 participants
@zapov
Contributor

zapov commented Sep 17, 2017

If an argument is larger than 200MB it would overflow during escaping due to optimistic size estimation.
Change size estimation so it works for even larger arguments until StringBuilder crashes with negative size error.

@codecov-io

This comment has been minimized.

codecov-io commented Sep 17, 2017

Codecov Report

Merging #946 into master will increase coverage by 0.61%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #946      +/-   ##
============================================
+ Coverage     65.93%   66.55%   +0.61%     
- Complexity     3560     3740     +180     
============================================
  Files           166      167       +1     
  Lines         15244    16196     +952     
  Branches       2465     2758     +293     
============================================
+ Hits          10051    10779     +728     
- Misses         4022     4222     +200     
- Partials       1171     1195      +24
@davecramer

This comment has been minimized.

Member

davecramer commented Sep 17, 2017

looks like this finds a bug in the jdk6 buffer overflow

@sehrope

This comment has been minimized.

Contributor

sehrope commented Sep 17, 2017

Nice catch! I hope for your sake you're not loading 200+MB literals into your DB though!

Regarding the patch, is there a noticeable perf or GC difference between estimating the 10% increase in size or just letting the JVM's implementation of StringBuilder deal with it? i.e. as long as we're changing this why not get rid of the guess entirely?

FYI, I get the +2 as the identifier quoting would be at least 2 chars longer for the quotes but I bet the common case for literals would be ones that don't need any escaping at all.

@zapov

This comment has been minimized.

Contributor

zapov commented Sep 18, 2017

@davecramer do you want me to try using long casting instead?

@sehrope well, our production failed on this, due to logging from the driver (which I was not even aware was enabled). So 10% seems sane for that case, since it's not string literal.

This is not really a big problem for me (I've fixed the DB invocation to use chunking) and I will turn off the logging since I don't need it (especially not the finest level)

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 18, 2017

This is not really a big problem for me (I've fixed the DB invocation to use chunking) and I will turn off the logging since I don't need it (especially not the finest level)

@zapov , could you elaborate more on the cause of "logging being enabled"?
I think it might silently hurt lots of users, so I wonder if there's some "common case for logging being enabled". At the end of the day, pgjdbc "finest" logging was supposed to be activated on a on-demand only basis.

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 18, 2017

do you want me to try using long casting instead?

I think the current fix is just fine.

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 25, 2017

@zapov, could you please add a test case to trigger/test the issue?

@zapov

This comment has been minimized.

Contributor

zapov commented Sep 25, 2017

@vlsi I'll look into why logging was enabled probably next week. I did not write a test for it since it would require allocating several > 200MB as strings .... which sounds like it would break a lot of stuff
Can do it next week if you still want it, but I don't think it's a good idea

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 25, 2017

I did not write a test for it since it would require allocating several > 200MB as strings .... which sounds like it would break a lot of stuff

Frankly speaking, I've seen a real case for 500MiB of binds being sent to Oracle DB in one go.

I'm inclined to test stuff otherwise the fix can be "reverted" by accident (e.g. as a part of a refactoring or something like that).

I'm not sure how long it would take Travis to perform select ? with a 200MiB literal.

@jorsol

This comment has been minimized.

Contributor

jorsol commented Sep 25, 2017

@zapov , If the test is really slow, I think @Ignored would be fine, that way, it can be tested locally from time to time.

And maybe later can be added a TAG to mark it as slow and just run it in a single job from Travis.

@zapov zapov force-pushed the zapov:escape-overflow branch from 348c7bb to 02e0b54 Oct 4, 2017

@zapov

This comment has been minimized.

Contributor

zapov commented Oct 4, 2017

I've added the test (they take +10secs each)

I've also looked into why this was even enabled and it seems Play turns it on during initialization via

Option(java.util.logging.Logger.getLogger("")).map { root =>
  root.setLevel(Level.FINEST)
  root.getHandlers.foreach(root.removeHandler(_))
}

@zapov zapov force-pushed the zapov:escape-overflow branch 2 times, most recently from 783a366 to 4b68383 Oct 4, 2017

fix: Avoid integer overflow for large arguments
If an argument is larger than 200MB it would overflow during escaping due to optimistic size estimation.
Change size estimation so it works for even larger arguments until StringBuilder crashes with negative size error.

@zapov zapov force-pushed the zapov:escape-overflow branch from 4b68383 to 7c657a1 Oct 4, 2017

@vlsi vlsi added this to the 42.1.5 milestone Oct 24, 2017

@vlsi vlsi merged commit 266ed61 into pgjdbc:master Oct 24, 2017

2 checks passed

codecov/project 66.55% (+0.61%) compared to 3286c8c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vlsi vlsi modified the milestones: 42.1.5, 42.2.0 Jan 8, 2018

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

fix: avoid integer overflow when sending large arguments (pgjdbc#946)
If an argument is larger than 200MB it would overflow during escaping due to optimistic size estimation.
Change size estimation so it works for even larger arguments until StringBuilder crashes with negative size error.

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

fix: avoid integer overflow when sending large arguments (pgjdbc#946)
If an argument is larger than 200MB it would overflow during escaping due to optimistic size estimation.
Change size estimation so it works for even larger arguments until StringBuilder crashes with negative size error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment