Reading big WKT geometry cause "memory access out of bounds" in Chrome and "zsh: bus error" in Node env #26
Comments
|
@chrispahm Since WKT geometry (and other WKB Hex and GeoJSON) size can be further bigger, so I think that using heap memory by But changing diff --git a/src/allCFunctions.mjs b/src/allCFunctions.mjs
index 3d5aca7..bef3ba7 100644
--- a/src/allCFunctions.mjs
+++ b/src/allCFunctions.mjs
@@ -3185,7 +3185,17 @@ pointer * @param {number} s - The coordinate sequence pointer
* @see {@link https://libgeos.org/doxygen/geos__c_8h.html#a0a0f7c1b9f6a9f7c3c4d1b5a7b6f9e2e|GEOSWKTReader_read_r}
* @alias module:geos
*/
- geos.GEOSWKTReader_read = null
+ geos.GEOSWKTReader_read = (reader, wkt) => {
+ if (typeof wkt !== 'string' || wkt.length === 0) {
+ return 0
+ }
+ const size = wkt.length + 1
+ const wktPtr = Module._malloc(size)
+ Module.stringToUTF8(wkt, wktPtr, size)
+ const geomPtr = geos.GEOSWKTReader_read_r(geos._ctx, reader, wktPtr)
+ Module._free(wktPtr)
+ return geomPtr
+ }
/**
* Reads a WKT string and returns a GEOSGeometry object.
@@ -3197,7 +3207,7 @@ pointer * @param {number} s - The coordinate sequence pointer
* @see {@link https://libgeos.org/doxygen/geos__c_8h.html#a0a0f7c1b9f6a9f7c3c4d1b5a7b6f9e2e|GEOSWKTReader_read_r}
* @alias module:geos
*/
- geos.GEOSWKTReader_read_r = Module.cwrap('GEOSWKTReader_read_r', 'number', ['number', 'number', 'string'])
+ geos.GEOSWKTReader_read_r = Module.cwrap('GEOSWKTReader_read_r', 'number', ['number', 'number', 'number'])
/**
* Creates a GEOSWKTWriter object.
@@ -3238,7 +3248,12 @@ pointer * @param {number} s - The coordinate sequence pointer
* @returns {string} The WKT representation of the geometry, or null on error.
* @alias module:geos
*/
- geos.GEOSWKTWriter_write = null
+ geos.GEOSWKTWriter_write = (writer, g) => {
+ const wktPtr = geos.GEOSWKTWriter_write_r(geos._ctx, writer, g)
+ const wkt = Module.UTF8ToString(wktPtr)
+ geos.GEOSFree_r(geos._ctx, wktPtr)
+ return wkt
+ }
/**
* Converts a GEOSGeometry object to a WKT string using a context handle.
@@ -3248,7 +3263,7 @@ pointer * @param {number} s - The coordinate sequence pointer
* @returns {string} The WKT representation of the geometry, or null on error.
* @alias module:geos
*/
- geos.GEOSWKTWriter_write_r = Module.cwrap('GEOSWKTWriter_write_r', 'string', ['number', 'number', 'number'])
+ geos.GEOSWKTWriter_write_r = Module.cwrap('GEOSWKTWriter_write_r', 'number', ['number', 'number', 'number'])
/**
* Sets whether the output WKT string should be trimmed of unnecessary spaces.
@@ -5135,7 +5150,9 @@ pointer * @param {number} s - The coordinate sequence pointer
Object.keys(geos).forEach(property => {
if (typeof geos[property] === 'function' && property.endsWith('_r')) {
const nonReFunctionName = property.slice(0, -2)
- geos[nonReFunctionName] = (...args) => geos[property](geos._ctx, ...args)
+ if (typeof geos[nonReFunctionName] !== 'function') {
+ geos[nonReFunctionName] = (...args) => geos[property](geos._ctx, ...args)
+ }
}
})How do you think about above ? |
|
I'm wondering if it's really necessary/ a good idea to keep the current API, since a) we would have to keep maintaining it in the future and b) other users will run into the memory access out of bounds issue and would then need to manually find out how to fix that! Since there are not many users of this library (according to npm), I think we could also introduce a breaking change and signal that the API changed by releasing a new major version (as pointed out by https://semver.org/ "MAJOR version when you make incompatible API changes"). What do you think? I guess it would make our lives easier as all we would have to change would be a few changes of "string" → "number" in the |
|
@chrispahm Thanks for reply!
Okay, it sounds good for me to upgrade major version for breaking API changes. 👍
Okay. I guess that some functions (ex. Otherwise, it is okay to replace "string" to "number", but WKT reader/writer called amount is not a few in the tests at this moment,
and replacing the all calls to raw pointer access will increase the test code, so something adding helper methods (which are similar with original |
|
@sanak @chrispahm any plan to fix this issue? because we are using this library for geometry validation and getting this error a lot. |
|
Hi @chrispahm, @mtrcn At this moment, there are 3 patterns to fix this issue.
But, do you have a preference from above No.1~3 or another idea ? From next week, I will have a time, so I am okay to create PR about this (after #28, #29 completion). |
|
Hello @sanak, I think the first option seems more future-proof to me but it will require you to update tests as I understand. |
|
@chrispahm @mtrcn
Well, once it was done, updating tests will not be so hard. 😊
Oh, yes. 😊 |
Change WKT Reader/Writer 'string' type to 'number' (#26)
|
Alright, just published v2.0.0 on npm! Thanks @sanak for providing the fix and everyone else for the discussion! |
Chrome env error is reproducible from the following step.
misc-fme.xmlfromTestXml, then clickLoadbutton# 1 - Big Bad Nasty bufferfromTestCaseNode env error is reproducible from the following step.
test/tests/foldernpx tape test/tests/wkt_big_geometry.spec.mjsFrom the following changes, I could solve above issue.
Change WKT Reader/Writer
stringtype tonumberRun
npm run buildSwitch to old WKT read/write style
Run
npx tape test/tests/wkt_big_geometry.spec.mjsThe text was updated successfully, but these errors were encountered: