You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by "leerho (via GitHub)" <gi...@apache.org> on 2024/02/16 00:57:15 UTC
[PR] All except 2 of the fixes here were security related fixes, (datasketches-java)
leerho opened a new pull request, #509:
URL: https://github.com/apache/datasketches-java/pull/509
to harden our classes against "finalizer attacks".
See https://wiki.sei.cmu.edu/confluence/display/java/OBJ11-J.+Be+wary+of+letting+constructors+throw+exceptions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] These fixes address security related issues. (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#discussion_r1506846577
##########
src/main/java/org/apache/datasketches/tuple/CompactSketch.java:
##########
@@ -109,13 +111,17 @@ private enum Flags { IS_BIG_ENDIAN, IS_READ_ONLY, IS_EMPTY, IS_COMPACT, IS_ORDER
offset += classNameLength;
}
hashArr_ = new long[count];
+
for (int i = 0; i < count; i++) {
hashArr_[i] = mem.getLong(offset);
offset += Long.BYTES;
}
for (int i = 0; i < count; i++) {
offset += readSummary(mem, offset, i, count, deserializer);
}
+ } else {
Review Comment:
This was caught by SpotBugs that needed to be final, which means it needs to be initialized whether the sketch has entries or not. summaryArr_ is along for the ride, just to make sure it has a valid initialization too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] These fixes address security related issues. (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#discussion_r1506846577
##########
src/main/java/org/apache/datasketches/tuple/CompactSketch.java:
##########
@@ -109,13 +111,17 @@ private enum Flags { IS_BIG_ENDIAN, IS_READ_ONLY, IS_EMPTY, IS_COMPACT, IS_ORDER
offset += classNameLength;
}
hashArr_ = new long[count];
+
for (int i = 0; i < count; i++) {
hashArr_[i] = mem.getLong(offset);
offset += Long.BYTES;
}
for (int i = 0; i < count; i++) {
offset += readSummary(mem, offset, i, count, deserializer);
}
+ } else {
Review Comment:
This was caught by SpotBugs that `hashArr_` needed to be final, which means it needs to be initialized whether the sketch has entries or not. `summaryArr_` is along for the ride, just to make sure it has a valid initialization too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] These fixes address security related issues. (datasketches-java)
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#discussion_r1505177193
##########
src/test/java/org/apache/datasketches/tuple/strings/ArrayOfStringsSketchTest.java:
##########
@@ -46,11 +46,11 @@ public void checkSketch() {
sketch1.update(strArrArr[i], strArrArr[i]);
}
sketch1.update(strArrArr[0], strArrArr[0]); //insert duplicate
- //printSummaries(sketch1.iterator());
+ printSummaries(sketch1.iterator());
Review Comment:
did you mean to add this back in? (yes is fine, but if no just wanna flag the)
##########
src/main/java/org/apache/datasketches/theta/DirectQuickSelectSketch.java:
##########
@@ -106,8 +107,29 @@
final MemoryRequestServer memReqSvr,
final WritableMemory dstMem,
final boolean unionGadget) {
- super(seed, dstMem);
+ this(
+ checkMemSize(lgNomLongs, rf, dstMem, unionGadget),
+ //SpotBugs CT_CONSTRUCTOR_THROW is false positive.
+ //this construction scheme is compliant with SEI CERT Oracle Coding Standard for Java / OBJ11-J
+ lgNomLongs,
+ seed,
+ p,
+ rf,
+ memReqSvr,
+ dstMem,
+ unionGadget);
+ }
+ private DirectQuickSelectSketch(
+ final boolean secure,
Review Comment:
Add a quick note for future reference, even though it's documented just above
##########
src/main/java/org/apache/datasketches/tuple/CompactSketch.java:
##########
@@ -109,13 +111,17 @@ private enum Flags { IS_BIG_ENDIAN, IS_READ_ONLY, IS_EMPTY, IS_COMPACT, IS_ORDER
offset += classNameLength;
}
hashArr_ = new long[count];
+
for (int i = 0; i < count; i++) {
hashArr_[i] = mem.getLong(offset);
offset += Long.BYTES;
}
for (int i = 0; i < count; i++) {
offset += readSummary(mem, offset, i, count, deserializer);
}
+ } else {
Review Comment:
This seems newly added. Did it fix a bug by handling a path that we previously ignored?
##########
src/main/java/org/apache/datasketches/quantiles/DoublesSketchAccessor.java:
##########
@@ -39,13 +40,30 @@
int numItems_;
int offset_;
- DoublesSketchAccessor(final DoublesSketch ds, final boolean forceSize, final int level) {
+ DoublesSketchAccessor(
+ final DoublesSketch ds,
+ final boolean forceSize,
+ final int level) {
+ this(checkLvl(level), ds, forceSize, level);
+ }
+
+ private DoublesSketchAccessor(
+ final boolean secure,
Review Comment:
Should add a comment (brief, just "helps avoid Finalizer Attack" is sufficient) for each of these instances.
##########
src/main/java/org/apache/datasketches/quantiles/ItemsSketch.java:
##########
@@ -36,14 +36,17 @@
import static org.apache.datasketches.quantiles.PreambleUtil.extractN;
import static org.apache.datasketches.quantiles.PreambleUtil.extractPreLongs;
import static org.apache.datasketches.quantiles.PreambleUtil.extractSerVer;
+//import static org.apache.datasketches.quantilescommon.QuantilesAPI.EMPTY_MSG;
Review Comment:
I think we can remove this?
##########
src/main/java/org/apache/datasketches/tuple/arrayofdoubles/DirectArrayOfDoublesQuickSelectSketch.java:
##########
@@ -61,12 +61,33 @@
* @param seed <a href="{@docRoot}/resources/dictionary.html#seed">See seed</a>
* @param dstMem <a href="{@docRoot}/resources/dictionary.html#mem">See Memory</a>
*/
- DirectArrayOfDoublesQuickSelectSketch(final int nomEntries, final int lgResizeFactor,
- final float samplingProbability, final int numValues, final long seed, final WritableMemory dstMem) {
+ DirectArrayOfDoublesQuickSelectSketch(
+ final int nomEntries,
+ final int lgResizeFactor,
+ final float samplingProbability,
+ final int numValues,
+ final long seed,
+ final WritableMemory dstMem) {
+ this(validate1(nomEntries, lgResizeFactor, numValues, dstMem),
+ nomEntries,
+ lgResizeFactor,
+ samplingProbability,
+ numValues,
+ seed,
+ dstMem);
+ }
+
+ private DirectArrayOfDoublesQuickSelectSketch(
+ final boolean secure,
Review Comment:
Please add a (very short) note here, and perhaps in the above constructor, too.
##########
src/main/java/org/apache/datasketches/tuple/arrayofdoubles/DirectArrayOfDoublesQuickSelectSketch.java:
##########
@@ -94,19 +115,52 @@ class DirectArrayOfDoublesQuickSelectSketch extends ArrayOfDoublesQuickSelectSke
setRebuildThreshold();
}
+ private static final boolean validate1(
Review Comment:
i feel like we can probably come up with better names :) but not a blocker, either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] These fixes address security related issues. (datasketches-java)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#discussion_r1506850022
##########
src/main/java/org/apache/datasketches/theta/DirectQuickSelectSketch.java:
##########
@@ -106,8 +107,29 @@
final MemoryRequestServer memReqSvr,
final WritableMemory dstMem,
final boolean unionGadget) {
- super(seed, dstMem);
+ this(
+ checkMemSize(lgNomLongs, rf, dstMem, unionGadget),
+ //SpotBugs CT_CONSTRUCTOR_THROW is false positive.
+ //this construction scheme is compliant with SEI CERT Oracle Coding Standard for Java / OBJ11-J
+ lgNomLongs,
+ seed,
+ p,
+ rf,
+ memReqSvr,
+ dstMem,
+ unionGadget);
+ }
+ private DirectQuickSelectSketch(
+ final boolean secure, //required part of Finalizer Attack prevention
Review Comment:
## Useless parameter
The parameter 'secure' is never used.
[Show more details](https://github.com/apache/datasketches-java/security/code-scanning/695)
##########
src/main/java/org/apache/datasketches/quantiles/DoublesSketchAccessor.java:
##########
@@ -39,13 +40,32 @@
int numItems_;
int offset_;
- DoublesSketchAccessor(final DoublesSketch ds, final boolean forceSize, final int level) {
+ DoublesSketchAccessor(
+ final DoublesSketch ds,
+ final boolean forceSize,
+ final int level) {
+ this(checkLvl(level), ds, forceSize, level);
+ //SpotBugs CT_CONSTRUCTOR_THROW is false positive.
+ //this construction scheme is compliant with SEI CERT Oracle Coding Standard for Java / OBJ11-J
+ }
+
+ private DoublesSketchAccessor(
+ final boolean secure, //required part of Finalizer Attack prevention
Review Comment:
## Useless parameter
The parameter 'secure' is never used.
[Show more details](https://github.com/apache/datasketches-java/security/code-scanning/694)
##########
src/main/java/org/apache/datasketches/tuple/arrayofdoubles/DirectArrayOfDoublesQuickSelectSketch.java:
##########
@@ -94,19 +117,52 @@
setRebuildThreshold();
}
+ private static final boolean checkMemory(
+ final int nomEntries,
+ final int lgResizeFactor,
+ final int numValues,
+ final WritableMemory dstMem) {
+ final int startingCapacity = Util.getStartingCapacity(nomEntries, lgResizeFactor);
+ checkIfEnoughMemory(dstMem, startingCapacity, numValues);
+ return true;
+ }
+
/**
* Wraps the given Memory.
* @param mem <a href="{@docRoot}/resources/dictionary.html#mem">See Memory</a>
* @param seed update seed
*/
- DirectArrayOfDoublesQuickSelectSketch(final WritableMemory mem, final long seed) {
+ DirectArrayOfDoublesQuickSelectSketch(
+ final WritableMemory mem,
+ final long seed) {
+ this(checkSerVer_Endianness(mem), mem, seed);
+ //SpotBugs CT_CONSTRUCTOR_THROW is false positive.
+ //this construction scheme is compliant with SEI CERT Oracle Coding Standard for Java / OBJ11-J
+ }
+
+ private DirectArrayOfDoublesQuickSelectSketch(
+ final boolean secure, //required part of Finalizer Attack prevention
Review Comment:
## Useless parameter
The parameter 'secure' is never used.
[Show more details](https://github.com/apache/datasketches-java/security/code-scanning/696)
##########
src/main/java/org/apache/datasketches/tuple/arrayofdoubles/DirectArrayOfDoublesQuickSelectSketch.java:
##########
@@ -61,12 +61,35 @@
* @param seed <a href="{@docRoot}/resources/dictionary.html#seed">See seed</a>
* @param dstMem <a href="{@docRoot}/resources/dictionary.html#mem">See Memory</a>
*/
- DirectArrayOfDoublesQuickSelectSketch(final int nomEntries, final int lgResizeFactor,
- final float samplingProbability, final int numValues, final long seed, final WritableMemory dstMem) {
+ DirectArrayOfDoublesQuickSelectSketch(
+ final int nomEntries,
+ final int lgResizeFactor,
+ final float samplingProbability,
+ final int numValues,
+ final long seed,
+ final WritableMemory dstMem) {
+ this(checkMemory(nomEntries, lgResizeFactor, numValues, dstMem),
+ //SpotBugs CT_CONSTRUCTOR_THROW is false positive.
+ //this construction scheme is compliant with SEI CERT Oracle Coding Standard for Java / OBJ11-J
+ nomEntries,
+ lgResizeFactor,
+ samplingProbability,
+ numValues,
+ seed,
+ dstMem);
+ }
+
+ private DirectArrayOfDoublesQuickSelectSketch(
+ final boolean secure, //required part of Finalizer Attack prevention
Review Comment:
## Useless parameter
The parameter 'secure' is never used.
[Show more details](https://github.com/apache/datasketches-java/security/code-scanning/697)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] These fixes address security related issues. (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho merged PR #509:
URL: https://github.com/apache/datasketches-java/pull/509
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] These fixes address security related issues. (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#discussion_r1506833840
##########
src/main/java/org/apache/datasketches/quantiles/ItemsSketch.java:
##########
@@ -36,14 +36,17 @@
import static org.apache.datasketches.quantiles.PreambleUtil.extractN;
import static org.apache.datasketches.quantiles.PreambleUtil.extractPreLongs;
import static org.apache.datasketches.quantiles.PreambleUtil.extractSerVer;
+//import static org.apache.datasketches.quantilescommon.QuantilesAPI.EMPTY_MSG;
Review Comment:
removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] These fixes address security related issues. (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#discussion_r1501651379
##########
src/main/java/org/apache/datasketches/theta/DirectQuickSelectSketch.java:
##########
@@ -106,8 +107,29 @@
final MemoryRequestServer memReqSvr,
final WritableMemory dstMem,
final boolean unionGadget) {
- super(seed, dstMem);
+ this(
+ checkMemSize(lgNomLongs, rf, dstMem, unionGadget),
+ //SpotBugs CT_CONSTRUCTOR_THROW is false positive.
+ //this construction scheme is compliant with SEI CERT Oracle Coding Standard for Java / OBJ11-J
+ lgNomLongs,
+ seed,
+ p,
+ rf,
+ memReqSvr,
+ dstMem,
+ unionGadget);
+ }
+ private DirectQuickSelectSketch(
+ final boolean secure,
Review Comment:
This "useless parameter" is actually a required part of the fix to avoid
Finalizer Attacks. See link at the top.
##########
src/main/java/org/apache/datasketches/tuple/arrayofdoubles/DirectArrayOfDoublesQuickSelectSketch.java:
##########
@@ -94,19 +115,52 @@
setRebuildThreshold();
}
+ private static final boolean validate1(
+ final int nomEntries,
+ final int lgResizeFactor,
+ final int numValues,
+ final WritableMemory dstMem) {
+ final int startingCapacity = Util.getStartingCapacity(nomEntries, lgResizeFactor);
+ checkIfEnoughMemory(dstMem, startingCapacity, numValues);
+ return true;
+ }
+
/**
* Wraps the given Memory.
* @param mem <a href="{@docRoot}/resources/dictionary.html#mem">See Memory</a>
* @param seed update seed
*/
- DirectArrayOfDoublesQuickSelectSketch(final WritableMemory mem, final long seed) {
+ DirectArrayOfDoublesQuickSelectSketch(
+ final WritableMemory mem,
+ final long seed) {
+ this(validate2(mem), mem, seed);
+ //SpotBugs CT_CONSTRUCTOR_THROW is false positive.
+ //this construction scheme is compliant with SEI CERT Oracle Coding Standard for Java / OBJ11-J
+ }
+
+ private DirectArrayOfDoublesQuickSelectSketch(
+ final boolean secure,
Review Comment:
This "useless parameter" is actually a required part of the fix to avoid
Finalizer Attacks. See link at the top.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] These fixes address security related issues. (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#issuecomment-1962435765
Fixing the Finalizer Attack Vulnerability across the entire library required several different approaches depending on the particular case. See the link at the top. Although it appears like a lot of added code, it actually isn't. In the case of the KLL and Classic quantiles Sorted Views, I had to move the code that created the SV from the SV class to the actual Sketch class.
In other cases, I had to create a new private constructor and move exception throwing code into a separate private static method. Where it was possible to make the class final, that is the simplest solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] All except 2 of the fixes here were security related fixes, (datasketches-java)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#discussion_r1491841247
##########
src/main/java/org/apache/datasketches/tuple/QuickSelectSketch.java:
##########
@@ -57,6 +57,11 @@
private long[] hashTable_;
S[] summaryTable_;
+ @Override
+ protected final void finalize() {
Review Comment:
## Finalizer inconsistency
Finalize in QuickSelectSketch nullifies finalize in QuickSelectSketch<>.
[Show more details](https://github.com/apache/datasketches-java/security/code-scanning/681)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] These fixes address security related issues. (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#discussion_r1501651653
##########
src/main/java/org/apache/datasketches/tuple/arrayofdoubles/DirectArrayOfDoublesQuickSelectSketch.java:
##########
@@ -61,12 +61,33 @@
* @param seed <a href="{@docRoot}/resources/dictionary.html#seed">See seed</a>
* @param dstMem <a href="{@docRoot}/resources/dictionary.html#mem">See Memory</a>
*/
- DirectArrayOfDoublesQuickSelectSketch(final int nomEntries, final int lgResizeFactor,
- final float samplingProbability, final int numValues, final long seed, final WritableMemory dstMem) {
+ DirectArrayOfDoublesQuickSelectSketch(
+ final int nomEntries,
+ final int lgResizeFactor,
+ final float samplingProbability,
+ final int numValues,
+ final long seed,
+ final WritableMemory dstMem) {
+ this(validate1(nomEntries, lgResizeFactor, numValues, dstMem),
+ nomEntries,
+ lgResizeFactor,
+ samplingProbability,
+ numValues,
+ seed,
+ dstMem);
+ }
+
+ private DirectArrayOfDoublesQuickSelectSketch(
+ final boolean secure,
Review Comment:
This "useless parameter" is actually a required part of the fix to avoid
Finalizer Attacks. See link at the top.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] These fixes address security related issues. (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#discussion_r1506840713
##########
src/main/java/org/apache/datasketches/tuple/arrayofdoubles/DirectArrayOfDoublesQuickSelectSketch.java:
##########
@@ -94,19 +115,52 @@ class DirectArrayOfDoublesQuickSelectSketch extends ArrayOfDoublesQuickSelectSke
setRebuildThreshold();
}
+ private static final boolean validate1(
Review Comment:
fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] All except 2 of the fixes here were security related fixes, (datasketches-java)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#discussion_r1501028837
##########
src/main/java/org/apache/datasketches/theta/DirectQuickSelectSketch.java:
##########
@@ -106,8 +107,29 @@
final MemoryRequestServer memReqSvr,
final WritableMemory dstMem,
final boolean unionGadget) {
- super(seed, dstMem);
+ this(
+ checkMemSize(lgNomLongs, rf, dstMem, unionGadget),
+ //SpotBugs CT_CONSTRUCTOR_THROW is false positive.
+ //this construction scheme is compliant with SEI CERT Oracle Coding Standard for Java / OBJ11-J
+ lgNomLongs,
+ seed,
+ p,
+ rf,
+ memReqSvr,
+ dstMem,
+ unionGadget);
+ }
+ private DirectQuickSelectSketch(
+ final boolean secure,
Review Comment:
## Useless parameter
The parameter 'secure' is never used.
[Show more details](https://github.com/apache/datasketches-java/security/code-scanning/686)
##########
src/main/java/org/apache/datasketches/tuple/arrayofdoubles/DirectArrayOfDoublesQuickSelectSketch.java:
##########
@@ -61,12 +61,33 @@
* @param seed <a href="{@docRoot}/resources/dictionary.html#seed">See seed</a>
* @param dstMem <a href="{@docRoot}/resources/dictionary.html#mem">See Memory</a>
*/
- DirectArrayOfDoublesQuickSelectSketch(final int nomEntries, final int lgResizeFactor,
- final float samplingProbability, final int numValues, final long seed, final WritableMemory dstMem) {
+ DirectArrayOfDoublesQuickSelectSketch(
+ final int nomEntries,
+ final int lgResizeFactor,
+ final float samplingProbability,
+ final int numValues,
+ final long seed,
+ final WritableMemory dstMem) {
+ this(validate1(nomEntries, lgResizeFactor, numValues, dstMem),
+ nomEntries,
+ lgResizeFactor,
+ samplingProbability,
+ numValues,
+ seed,
+ dstMem);
+ }
+
+ private DirectArrayOfDoublesQuickSelectSketch(
+ final boolean secure,
Review Comment:
## Useless parameter
The parameter 'secure' is never used.
[Show more details](https://github.com/apache/datasketches-java/security/code-scanning/688)
##########
src/main/java/org/apache/datasketches/tuple/arrayofdoubles/DirectArrayOfDoublesQuickSelectSketch.java:
##########
@@ -94,19 +115,52 @@
setRebuildThreshold();
}
+ private static final boolean validate1(
+ final int nomEntries,
+ final int lgResizeFactor,
+ final int numValues,
+ final WritableMemory dstMem) {
+ final int startingCapacity = Util.getStartingCapacity(nomEntries, lgResizeFactor);
+ checkIfEnoughMemory(dstMem, startingCapacity, numValues);
+ return true;
+ }
+
/**
* Wraps the given Memory.
* @param mem <a href="{@docRoot}/resources/dictionary.html#mem">See Memory</a>
* @param seed update seed
*/
- DirectArrayOfDoublesQuickSelectSketch(final WritableMemory mem, final long seed) {
+ DirectArrayOfDoublesQuickSelectSketch(
+ final WritableMemory mem,
+ final long seed) {
+ this(validate2(mem), mem, seed);
+ //SpotBugs CT_CONSTRUCTOR_THROW is false positive.
+ //this construction scheme is compliant with SEI CERT Oracle Coding Standard for Java / OBJ11-J
+ }
+
+ private DirectArrayOfDoublesQuickSelectSketch(
+ final boolean secure,
Review Comment:
## Useless parameter
The parameter 'secure' is never used.
[Show more details](https://github.com/apache/datasketches-java/security/code-scanning/687)
##########
src/main/java/org/apache/datasketches/quantiles/DoublesSketchAccessor.java:
##########
@@ -39,13 +40,30 @@
int numItems_;
int offset_;
- DoublesSketchAccessor(final DoublesSketch ds, final boolean forceSize, final int level) {
+ DoublesSketchAccessor(
+ final DoublesSketch ds,
+ final boolean forceSize,
+ final int level) {
+ this(checkLvl(level), ds, forceSize, level);
+ }
+
+ private DoublesSketchAccessor(
+ final boolean secure,
Review Comment:
## Useless parameter
The parameter 'secure' is never used.
[Show more details](https://github.com/apache/datasketches-java/security/code-scanning/685)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] These fixes address security related issues. (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#discussion_r1501650715
##########
src/main/java/org/apache/datasketches/quantiles/DoublesSketchAccessor.java:
##########
@@ -39,13 +40,30 @@
int numItems_;
int offset_;
- DoublesSketchAccessor(final DoublesSketch ds, final boolean forceSize, final int level) {
+ DoublesSketchAccessor(
+ final DoublesSketch ds,
+ final boolean forceSize,
+ final int level) {
+ this(checkLvl(level), ds, forceSize, level);
+ }
+
+ private DoublesSketchAccessor(
+ final boolean secure,
Review Comment:
This "useless parameter" is actually a required part of the fix to avoid
Finalizer Attacks. See link at the top.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
Re: [PR] These fixes address security related issues. (datasketches-java)
Posted by "leerho (via GitHub)" <gi...@apache.org>.
leerho commented on code in PR #509:
URL: https://github.com/apache/datasketches-java/pull/509#discussion_r1506842112
##########
src/test/java/org/apache/datasketches/tuple/strings/ArrayOfStringsSketchTest.java:
##########
@@ -46,11 +46,11 @@ public void checkSketch() {
sketch1.update(strArrArr[i], strArrArr[i]);
}
sketch1.update(strArrArr[0], strArrArr[0]); //insert duplicate
- //printSummaries(sketch1.iterator());
+ printSummaries(sketch1.iterator());
Review Comment:
Yes. Printing is disabled at the bottom of the class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org