Skip to content

Improve memory allocation and validation #401

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.

Sign up for GitHub

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

Open
Marcono1234 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve memory allocation and validation #401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 4 additions & 13 deletions src/main/java/org/apache/commons/imaging/bytesource/InputStreamByteSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@
import java.util.Objects;

import org.apache.commons.imaging.ImagingException;
import org.apache.commons.imaging.common.Allocator;
import org.apache.commons.imaging.common.BinaryFunctions;
import org.apache.commons.imaging.common.KnownSizeByteArrayBuilder;
import org.apache.commons.io.IOUtils;
import org.apache.commons.io.build.AbstractOrigin.InputStreamOrigin;

Expand Down Expand Up @@ -191,18 +191,9 @@ public byte[] getByteArray(final long position, final int length) throws IOExcep
final InputStream cis = getInputStream();
BinaryFunctions.skipBytes(cis, position);

final byte[] bytes = Allocator.byteArray(length);
int total = 0;
while (true) {
final int read = cis.read(bytes, total, bytes.length - total);
if (read < 1) {
throw new ImagingException("Could not read block.");
}
total += read;
if (total >= length) {
return bytes;
}
}
KnownSizeByteArrayBuilder byteArrayBuilder = new KnownSizeByteArrayBuilder(length);
byteArrayBuilder.addAllBytesFrom(cis);
return byteArrayBuilder.createByteArray();
}

private Block getFirstBlock() throws IOException {
Expand Down
213 changes: 158 additions & 55 deletions src/main/java/org/apache/commons/imaging/common/Allocator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

import java.math.BigInteger;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.function.IntFunction;

/**
Expand All @@ -36,23 +38,12 @@ public class Allocator {
LIMIT = Integer.getInteger(CANONICAL_NAME, DEFAULT);
}

/**
* Allocates an Object of type T of the requested size.
*
* @param <T> The return array type
* @param request The requested size.
* @param factory The array factory.
* @return a new byte array.
* @throws AllocationRequestException Thrown when the request exceeds the limit.
* @see #check(int)
*/
public static <T> T apply(final int request, final IntFunction<T> factory) {
return factory.apply(check(request));
}

/**
* Allocates an array of type T of the requested size.
*
* <p><b>Important:</b> If possible avoid this method and avoid Object arrays, and instead
* use {@link List}s created with {@link #arrayList(int)}.
*
* @param <T> The return array type
* @param request The requested size.
* @param factory The array factory.
Expand All @@ -62,22 +53,55 @@ public static <T> T apply(final int request, final IntFunction<T> factory) {
* @see #check(int)
*/
public static <T> T[] array(final int request, final IntFunction<T[]> factory, final int eltShallowByteSize) {
check(request * eltShallowByteSize);
check(Math.multiplyExact(request, eltShallowByteSize));
return factory.apply(request);
}

/**
* Allocates an Object array of type T of the requested size.
* Similar to {@link #array(int, IntFunction, int)}, except that the caller is reasonable sure that
* the requested size is 'trusted', that is:
* <ul>
* <li>It is a constant value which cannot be influenced by the user in any way
* <li>Or, it relates to the size of memory which has already been successfully allocated,
* for example an array of a different type or a {@link Collection}
* </ul>
*/
public static <T> T[] arrayTrusted(final int request, final IntFunction<T[]> factory, final int eltShallowByteSize) {
// For now simply delegate to `array`; this 'trusted' method here is at the moment
// mainly intended to detect untrusted calls
return array(request, factory, eltShallowByteSize);
}

/**
* Creates an array list of the requested initial capacity. The capacity can be
* <i>untrusted</i>; that is, it may come from metadata which can directly be
* controlled by the (potentially malicious) user.
*/
public static <T> ArrayList<T> arrayList(int initialCapacity) {
// Limit the capacity to a reasonable maximum
return new ArrayList<>(Math.min(initialCapacity, 1024));
}

/**
* Creates an array list of the requested initial capacity. The capacity should be
* <i>trusted</i>; that is, it should either be a constant or there should already
* be an existing collection of the same size.
*
* @param <T> The return array type
* @param request The requested size.
* @return a new byte array.
* @throws AllocationRequestException Thrown when the request exceeds the limit.
* @see #check(int)
* <p>This method must not be called with with an untrusted capacity, for example
* one which has only been read from user-controlled metadata; use {@link #arrayList(int)}
* for that.
*/
public static <T> ArrayList<T> arrayList(final int request) {
check(24 + request * 4); // 4 bytes per element
return apply(request, ArrayList::new);
public static <T> ArrayList<T> arrayListTrusted(int initialCapacity) {
// For sanity still check requested capacity
check(Math.addExact(24, Math.multiplyExact(initialCapacity, 4))); // 4 bytes per element
return new ArrayList<>(initialCapacity);
}

/**
* Creates an array list with the size of the given collection as initial capacity.
*/
public static <T> ArrayList<T> arrayListWithCapacityFor(Collection<?> collection) {
return arrayListTrusted(collection.size());
}

/**
Expand All @@ -101,7 +125,22 @@ public static byte[] byteArray(final int request) {
* @see #check(int, int)
*/
public static byte[] byteArray(final long request) {
return new byte[check(request, Byte.BYTES)];
return byteArray(Math.toIntExact(request));
}

/**
* Similar to {@link #byteArray(int)}, except that the caller is reasonable sure that
* the requested size is 'trusted', that is:
* <ul>
* <li>It is a constant value which cannot be influenced by the user in any way
* <li>Or, it relates to the size of memory which has already been successfully allocated,
* for example an array of a different type or a {@link Collection}
* </ul>
*/
public static byte[] byteArrayTrusted(final int request) {
// For now simply delegate to `byteArray`; this 'trusted' method here is at the moment
// mainly intended to detect untrusted calls
return byteArray(request);
}

/**
Expand All @@ -116,18 +155,37 @@ public static char[] charArray(final int request) {
return new char[check(request, Character.BYTES)];
}

/**
* Similar to {@link #charArray(int)}, except that the caller is reasonable sure that
* the requested size is 'trusted', that is:
* <ul>
* <li>It is a constant value which cannot be influenced by the user in any way
* <li>Or, it relates to the size of memory which has already been successfully allocated,
* for example an array of a different type or a {@link Collection}
* </ul>
*/
public static char[] charArrayTrusted(final int request) {
// For now simply delegate to `charArray`; this 'trusted' method here is at the moment
// mainly intended to detect untrusted calls
return charArray(request);
}

/**
* Checks a request for meeting allocation limits.
* <p>
* The default limit is {@value #DEFAULT}, override with the system property
* "org.apache.commons.imaging.common.mylzw.AllocationChecker".
* "org.apache.commons.imaging.common.Allocator".
* </p>
*
* @param request an allocation request.
* @return the request.
* @throws AllocationRequestException Thrown when the request exceeds the limit.
*/
public static int check(final int request) {
// Check for numeric overflow from caller
if (request < 0) {
throw new IllegalArgumentException("Invalid request value: " + request);
}
if (request > LIMIT) {
throw new AllocationRequestException(LIMIT, request);
}
Expand All @@ -138,7 +196,7 @@ public static int check(final int request) {
* Checks a request for meeting allocation limits.
* <p>
* The default limit is {@value #DEFAULT}, override with the system property
* "org.apache.commons.imaging.common.mylzw.AllocationChecker".
* "org.apache.commons.imaging.common.Allocator".
* </p>
*
* @param request an allocation request count.
Expand All @@ -147,6 +205,11 @@ public static int check(final int request) {
* @throws AllocationRequestException Thrown when the request exceeds the limit.
*/
public static int check(final int request, final int elementSize) {
// Check for numeric overflow from caller
if (request < 0) {
throw new IllegalArgumentException("Invalid request value: " + request);
}

int multiplyExact;
try {
multiplyExact = Math.multiplyExact(request, elementSize);
Expand All @@ -159,26 +222,6 @@ public static int check(final int request, final int elementSize) {
return request;
}

/**
* Checks a request for meeting allocation limits.
* <p>
* The default limit is {@value #DEFAULT}, override with the system property
* "org.apache.commons.imaging.common.mylzw.AllocationChecker".
* </p>
*
* @param request an allocation request count is cast down to an int.
* @param elementSize The element size.
* @return the request.
* @throws AllocationRequestException Thrown when the request exceeds the limit.
*/
public static int check(final long request, final int elementSize) {
try {
return check(Math.toIntExact(request), elementSize);
} catch (ArithmeticException e) {
throw new AllocationRequestException(LIMIT, request, e);
}
}

/**
* Checks that allocating a byte array of the requested size is within the limit.
*
Expand All @@ -189,6 +232,19 @@ public static int checkByteArray(final int request) {
return check(request, Byte.BYTES);
}

/**
* Similar to {@link #checkByteArray(int)}, except that the caller is reasonable sure that
* the requested size is 'trusted', that is:
* <ul>
* <li>It is a constant value which cannot be influenced by the user in any way
* <li>Or, it relates to the size of memory which has already been successfully allocated,
* for example an array of a different type or a {@link Collection}
* </ul>
*/
public static int checkByteArrayTrusted(final int request) {
return checkByteArray(request);
}

/**
* Allocates a double array of the requested size.
*
Expand All @@ -201,6 +257,21 @@ public static double[] doubleArray(final int request) {
return new double[check(request, Double.BYTES)];
}

/**
* Similar to {@link #doubleArray(int)}, except that the caller is reasonable sure that
* the requested size is 'trusted', that is:
* <ul>
* <li>It is a constant value which cannot be influenced by the user in any way
* <li>Or, it relates to the size of memory which has already been successfully allocated,
* for example an array of a different type or a {@link Collection}
* </ul>
*/
public static double[] doubleArrayTrusted(final int request) {
// For now simply delegate to `doubleArray`; this 'trusted' method here is at the moment
// mainly intended to detect untrusted calls
return doubleArray(request);
}

/**
* Allocates a float array of the requested size.
*
Expand All @@ -213,6 +284,21 @@ public static float[] floatArray(final int request) {
return new float[check(request, Float.BYTES)];
}

/**
* Similar to {@link #floatArray(int)}, except that the caller is reasonable sure that
* the requested size is 'trusted', that is:
* <ul>
* <li>It is a constant value which cannot be influenced by the user in any way
* <li>Or, it relates to the size of memory which has already been successfully allocated,
* for example an array of a different type or a {@link Collection}
* </ul>
*/
public static float[] floatArrayTrusted(final int request) {
// For now simply delegate to `floatArray`; this 'trusted' method here is at the moment
// mainly intended to detect untrusted calls
return floatArray(request);
}

/**
* Allocates a int array of the requested size.
*
Expand All @@ -226,15 +312,18 @@ public static int[] intArray(final int request) {
}

/**
* Allocates a long array of the requested size.
*
* @param request The requested size.
* @return a new long array.
* @throws AllocationRequestException Thrown when the request exceeds the limit.
* @see #check(int, int)
* Similar to {@link #intArray(int)}, except that the caller is reasonable sure that
* the requested size is 'trusted', that is:
* <ul>
* <li>It is a constant value which cannot be influenced by the user in any way
* <li>Or, it relates to the size of memory which has already been successfully allocated,
* for example an array of a different type or a {@link Collection}
* </ul>
*/
public static long[] longArray(final int request) {
return new long[check(request, Long.BYTES)];
public static int[] intArrayTrusted(final int request) {
// For now simply delegate to `intArray`; this 'trusted' method here is at the moment
// mainly intended to detect untrusted calls
return intArray(request);
}

/**
Expand All @@ -249,4 +338,18 @@ public static short[] shortArray(final int request) {
return new short[check(request, Short.BYTES)];
}

/**
* Similar to {@link #shortArray(int)}, except that the caller is reasonable sure that
* the requested size is 'trusted', that is:
* <ul>
* <li>It is a constant value which cannot be influenced by the user in any way
* <li>Or, it relates to the size of memory which has already been successfully allocated,
* for example an array of a different type or a {@link Collection}
* </ul>
*/
public static short[] shortArrayTrusted(final int request) {
// For now simply delegate to `shortArray`; this 'trusted' method here is at the moment
// mainly intended to detect untrusted calls
return shortArray(request);
}
}
3 changes: 2 additions & 1 deletion src/main/java/org/apache/commons/imaging/common/BasicCParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,8 @@ public static String[] tokenizeRow(final String row) {
++numLiveTokens;
}
}
final String[] liveTokens = Allocator.array(numLiveTokens, String[]::new, 24);
// Trusted because length is based on length of existing array
final String[] liveTokens = Allocator.arrayTrusted(numLiveTokens, String[]::new, 24);
int next = 0;
for (final String token : tokens) {
if (token != null && !token.isEmpty()) {
Expand Down
Loading