Skip to content

Commit 406ae3c

Browse files
edburnsCopilot
andcommitted
Address Copilot review findings for FFI loader tests
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent c89a5e2 commit 406ae3c

4 files changed

Lines changed: 187 additions & 337 deletions

File tree

java/src/main/java/com/github/copilot/ffi/NativeRuntimeLoader.java

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@
2424
* <p>
2525
* Resolution order:
2626
* <ol>
27-
* <li>The {@code COPILOT_CLI_PATH} environment variable (if set, treated as the
28-
* resolved path and returned directly).</li>
27+
* <li>The {@code COPILOT_RUNTIME_PATH} environment variable (if set, treated as
28+
* the resolved {@code runtime.node} path and returned directly).</li>
2929
* <li>Classpath resource {@code native/<classifier>/runtime.node} extracted to
3030
* {@code ~/.copilot/runtime-cache/<version>/<classifier>/runtime.node}.</li>
3131
* <li>A {@code runtime.node} file alongside the bundled CLI binary.</li>
@@ -49,6 +49,8 @@ public final class NativeRuntimeLoader {
4949
private static final Logger LOG = Logger.getLogger(NativeRuntimeLoader.class.getName());
5050
private static final String PROPERTIES_RESOURCE = "copilot-runtime.properties";
5151
private static final String BINARY_NAME = "runtime.node";
52+
private static final String RUNTIME_PATH_ENV = "COPILOT_RUNTIME_PATH";
53+
private static final String CLI_PATH_ENV = "COPILOT_CLI_PATH";
5254

5355
private NativeRuntimeLoader() {
5456
}
@@ -62,26 +64,30 @@ private NativeRuntimeLoader() {
6264
* if the binary cannot be resolved, extracted, or cached
6365
*/
6466
public static Path resolve() throws NativeRuntimeLoaderException {
65-
// 1. COPILOT_CLI_PATH override
66-
String cliPathEnv = System.getenv("COPILOT_CLI_PATH");
67-
if (cliPathEnv != null && !cliPathEnv.isBlank()) {
68-
return Paths.get(cliPathEnv);
67+
return resolve(System.getenv(RUNTIME_PATH_ENV), System.getenv(CLI_PATH_ENV),
68+
NativeRuntimeLoader.class.getClassLoader(), Paths.get(System.getProperty("user.home")),
69+
PlatformDetector.detectClassifier());
70+
}
71+
72+
static Path resolve(String runtimePathOverride, String bundledCliPath, ClassLoader classLoader, Path userHome,
73+
String classifier) throws NativeRuntimeLoaderException {
74+
// 1. Explicit runtime.node override
75+
if (runtimePathOverride != null && !runtimePathOverride.isBlank()) {
76+
return Paths.get(runtimePathOverride);
6977
}
7078

7179
// 2. Extract from classpath resource
72-
String version = loadVersion();
73-
String classifier = PlatformDetector.detectClassifier();
7480
String resourcePath = "native/" + classifier + "/" + BINARY_NAME;
7581

76-
URL resourceUrl = NativeRuntimeLoader.class.getClassLoader().getResource(resourcePath);
82+
URL resourceUrl = classLoader.getResource(resourcePath);
7783
if (resourceUrl != null) {
78-
return extractToCache(resourceUrl, version, classifier);
84+
String version = loadVersion(classLoader);
85+
return extractToCache(resourceUrl, version, classifier, userHome);
7986
}
8087

8188
// 3. Alongside bundled CLI (fall-through when no classpath resource)
82-
String bundledCli = System.getenv("COPILOT_CLI_PATH");
83-
if (bundledCli != null && !bundledCli.isBlank()) {
84-
Path sibling = Paths.get(bundledCli).getParent();
89+
if (bundledCliPath != null && !bundledCliPath.isBlank()) {
90+
Path sibling = Paths.get(bundledCliPath).getParent();
8591
if (sibling != null) {
8692
Path candidate = sibling.resolve(BINARY_NAME);
8793
if (isValidCacheEntry(candidate)) {
@@ -103,7 +109,11 @@ public static Path resolve() throws NativeRuntimeLoaderException {
103109
* if the resource is missing or the version value is blank
104110
*/
105111
static String loadVersion() throws NativeRuntimeLoaderException {
106-
InputStream in = NativeRuntimeLoader.class.getClassLoader().getResourceAsStream(PROPERTIES_RESOURCE);
112+
return loadVersion(NativeRuntimeLoader.class.getClassLoader());
113+
}
114+
115+
static String loadVersion(ClassLoader classLoader) throws NativeRuntimeLoaderException {
116+
InputStream in = classLoader.getResourceAsStream(PROPERTIES_RESOURCE);
107117
if (in == null) {
108118
throw new NativeRuntimeLoaderException("Missing classpath resource: " + PROPERTIES_RESOURCE
109119
+ ". Ensure the SDK JAR was built with Maven resource filtering enabled.");
@@ -123,9 +133,9 @@ static String loadVersion() throws NativeRuntimeLoaderException {
123133
return version.trim();
124134
}
125135

126-
private static Path extractToCache(URL resourceUrl, String version, String classifier)
136+
static Path extractToCache(URL resourceUrl, String version, String classifier, Path userHome)
127137
throws NativeRuntimeLoaderException {
128-
Path cacheDir = Paths.get(System.getProperty("user.home"), ".copilot", "runtime-cache", version, classifier);
138+
Path cacheDir = userHome.resolve(Paths.get(".copilot", "runtime-cache", version, classifier));
129139
Path cached = cacheDir.resolve(BINARY_NAME);
130140

131141
// 1. Cache hit: regular, non-empty file

java/src/main/java/com/github/copilot/ffi/PlatformDetector.java

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -151,15 +151,19 @@ public static LinuxLibc detectLinuxLibc() {
151151
* if the platform is not recognised or not supported
152152
*/
153153
public static String detectClassifier() {
154-
String os = detectOs();
155-
String arch = detectArch();
156-
if (!"linux".equals(os)) {
157-
String classifier = os + "-" + arch;
158-
validateClassifier(classifier);
159-
return classifier;
154+
return detectClassifier(System.getProperty("os.name", ""), System.getProperty("os.arch", ""),
155+
detectLinuxLibc());
156+
}
157+
158+
static String detectClassifier(String osName, String osArch, LinuxLibc linuxLibc) {
159+
String os = detectOs(osName);
160+
String arch = detectArch(osArch);
161+
String classifier;
162+
if ("linux".equals(os)) {
163+
classifier = (linuxLibc == LinuxLibc.MUSL ? "linuxmusl-" : "linux-") + arch;
164+
} else {
165+
classifier = os + "-" + arch;
160166
}
161-
LinuxLibc libc = detectLinuxLibc();
162-
String classifier = (libc == LinuxLibc.MUSL ? "linuxmusl-" : "linux-") + arch;
163167
validateClassifier(classifier);
164168
return classifier;
165169
}
@@ -230,16 +234,22 @@ static String readElfPtInterp(Path executablePath) throws IOException {
230234
if (phentsize <= 0 || phnum <= 0) {
231235
throw new IOException("Invalid ELF program header metadata: phentsize=" + phentsize + ", phnum=" + phnum);
232236
}
237+
int minProgramHeaderSize = elfClass == ELF_CLASS_64 ? 56 : 32;
238+
if (phentsize < minProgramHeaderSize) {
239+
throw new IOException("Invalid ELF program header entry size: " + phentsize + " (expected >= "
240+
+ minProgramHeaderSize + ")");
241+
}
233242

234243
for (int i = 0; i < phnum; i++) {
235244
long baseLong = phoff + ((long) i * phentsize);
236245
if (baseLong < 0 || baseLong > Integer.MAX_VALUE) {
237246
break;
238247
}
239-
int base = (int) baseLong;
240-
if (base + phentsize > size) {
248+
long entryEnd = baseLong + phentsize;
249+
if (entryEnd > size) {
241250
break;
242251
}
252+
int base = (int) baseLong;
243253

244254
long pType = readUInt32(probe, base, littleEndian);
245255
if (pType != PT_INTERP) {
@@ -260,11 +270,12 @@ static String readElfPtInterp(Path executablePath) throws IOException {
260270
throw new IOException("Invalid PT_INTERP bounds");
261271
}
262272

263-
int start = (int) pOffset;
264-
int end = start + (int) pFileSize;
265-
if (end > size) {
273+
long endLong = pOffset + pFileSize;
274+
if (endLong > size || endLong > Integer.MAX_VALUE || endLong <= pOffset) {
266275
throw new IOException("PT_INTERP extends past probe window; increase probe size");
267276
}
277+
int start = (int) pOffset;
278+
int end = (int) endLong;
268279

269280
int nulIndex = start;
270281
while (nulIndex < end && probe[nulIndex] != 0) {

0 commit comments

Comments
 (0)