Skip to content

Commit 7ab6aa1

Browse files
ekharkunovekharkunov
authored
Codeql improvements (#922)
* Introduced separate immutable field to lock access to currentCacheDir in Cocoapods service * Fixed formating placeholders. Removed redundant condition * Manifest merge tool argument parsing was refactored. Fixed codeql reports in that area * Removed unused boxed value usage * Removed unused boxed value usage * Handled NumberFormattingException. Stripped symbolic postfix from pod version when doing comparasion
1 parent 6e6d5c3 commit 7ab6aa1

11 files changed

Lines changed: 200 additions & 58 deletions

File tree

‎client/src/main/java/com/defold/extender/client/ExtenderClientCache.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,10 @@ public ExtenderClientCache(File cacheDir) throws IOException {
3535
*/
3636
public String getHash(ExtenderResource extenderResource) throws ExtenderClientException {
3737
String path = extenderResource.getPath();
38-
Long fileTimestamp = extenderResource.getLastModified();
38+
long fileTimestamp = extenderResource.getLastModified();
3939
Long timestamp = this.timestamps.get(path);
4040

41-
if (timestamp != null && fileTimestamp.equals(timestamp) ) {
41+
if (timestamp != null && timestamp.longValue() == fileTimestamp) {
4242
String hash = this.hashes.get(path);
4343
if (hash != null) {
4444
return hash;
@@ -180,7 +180,7 @@ private static String hash(ExtenderResource extenderResource) throws ExtenderCli
180180
md.update(data);
181181
return hashToString(md.digest());
182182
} catch(Exception e){
183-
throw new ExtenderClientException(String.format("Failed to hash resource: ", extenderResource.getPath()), e);
183+
throw new ExtenderClientException(String.format("Failed to hash resource: %s", extenderResource.getPath()), e);
184184
}
185185
}
186186

‎server/manifestmergetool/src/main/java/com/defold/manifestmergetool/ManifestMergeTool.java‎

Lines changed: 57 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -92,48 +92,73 @@ public static void merge(Platform platform, File main, File output, List<File> l
9292
logger.log(Level.FINE, "Merging done");
9393
}
9494

95-
/**
96-
* Merges a main manifest with several stubs
97-
*/
98-
public static void main(String[] args) throws Exception {
99-
95+
static class ParsedArgs {
96+
Platform platform = Platform.UNKNOWN;
10097
File main = null;
10198
File output = null;
10299
List<File> libraries = new ArrayList<>();
100+
}
103101

104-
Platform platform = Platform.UNKNOWN;
102+
static ParsedArgs parseArgs(String[] args) {
103+
if ((args.length % 2) != 0) {
104+
throw new IllegalArgumentException("Expected key/value argument pairs, got odd number of arguments: " + args.length);
105+
}
105106

106-
int index = 0;
107-
for (int i = 0; i < args.length; ++i) {
108-
if (args[i].equals("--main") && (index+1) < args.length) {
109-
main = new File(args[++i]);
110-
}
111-
else if (args[i].equals("--out") && (index+1) < args.length) {
112-
output = new File(args[++i]);
113-
}
114-
else if (args[i].equals("--lib") && (index+1) < args.length) {
115-
libraries.add(new File(args[++i]));
107+
ParsedArgs parsed = new ParsedArgs();
108+
for (int i = 0; i < args.length; i += 2) {
109+
String key = args[i];
110+
String value = args[i + 1];
111+
switch (key) {
112+
case "--main":
113+
parsed.main = new File(value);
114+
break;
115+
case "--out":
116+
parsed.output = new File(value);
117+
break;
118+
case "--lib":
119+
parsed.libraries.add(new File(value));
120+
break;
121+
case "--platform":
122+
switch (value) {
123+
case "android":
124+
parsed.platform = Platform.ANDROID;
125+
break;
126+
case "ios":
127+
parsed.platform = Platform.IOS;
128+
break;
129+
case "osx":
130+
parsed.platform = Platform.OSX;
131+
break;
132+
case "web":
133+
parsed.platform = Platform.WEB;
134+
break;
135+
default:
136+
throw new IllegalArgumentException(String.format("Unsupported platform: %s", value));
137+
}
138+
break;
139+
default:
140+
throw new IllegalArgumentException(String.format("Unknown argument: %s", key));
116141
}
142+
}
143+
return parsed;
144+
}
117145

118-
if (args[i].equals("--platform") && (index+1) < args.length) {
119-
++i;
120-
if (args[i].equals("android")) {
121-
platform = Platform.ANDROID;
122-
} else if (args[i].equals("ios")) {
123-
platform = Platform.IOS;
124-
} else if (args[i].equals("osx")) {
125-
platform = Platform.OSX;
126-
} else if (args[i].equals("web")) {
127-
platform = Platform.WEB;
128-
} else {
129-
ManifestMergeTool.logger.log(Level.SEVERE, String.format("Unsupported platform: %s", args[i]));
130-
System.exit(1);
131-
}
132-
}
146+
/**
147+
* Merges a main manifest with several stubs
148+
*/
149+
public static void main(String[] args) throws Exception {
150+
151+
ParsedArgs parsed;
152+
try {
153+
parsed = parseArgs(args);
154+
} catch (IllegalArgumentException e) {
155+
ManifestMergeTool.logger.log(Level.SEVERE, e.getMessage());
156+
System.exit(1);
157+
return;
133158
}
134159

135160
try {
136-
merge(platform, main, output, libraries);
161+
merge(parsed.platform, parsed.main, parsed.output, parsed.libraries);
137162
} catch(Exception e) {
138163
ManifestMergeTool.logger.log(Level.SEVERE, e.toString());
139164
System.exit(1);

‎server/manifestmergetool/src/test/java/com/defold/manifestmergetool/ManifestMergeToolTest.java‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
import java.nio.file.Files;
1010

1111
import static org.junit.jupiter.api.Assertions.assertEquals;
12+
import static org.junit.jupiter.api.Assertions.assertNull;
13+
import static org.junit.jupiter.api.Assertions.assertThrows;
1214
import static org.junit.jupiter.api.Assertions.assertTrue;
1315

1416
import org.apache.commons.io.FileUtils;
@@ -850,4 +852,89 @@ public void testMergeHTML5() throws IOException {
850852

851853
assertEquals(expected, merged);
852854
}
855+
856+
@Test
857+
public void testParseArgsEmpty() {
858+
ManifestMergeTool.ParsedArgs parsed = ManifestMergeTool.parseArgs(new String[]{});
859+
assertEquals(Platform.UNKNOWN, parsed.platform);
860+
assertNull(parsed.main);
861+
assertNull(parsed.output);
862+
assertTrue(parsed.libraries.isEmpty());
863+
}
864+
865+
@Test
866+
public void testParseArgsAllFlags() {
867+
String[] args = {
868+
"--platform", "android",
869+
"--main", "AndroidManifest.xml",
870+
"--lib", "lib1.xml",
871+
"--lib", "lib2.xml",
872+
"--out", "merged.xml",
873+
};
874+
ManifestMergeTool.ParsedArgs parsed = ManifestMergeTool.parseArgs(args);
875+
assertEquals(Platform.ANDROID, parsed.platform);
876+
assertEquals(new File("AndroidManifest.xml"), parsed.main);
877+
assertEquals(new File("merged.xml"), parsed.output);
878+
assertEquals(2, parsed.libraries.size());
879+
assertEquals(new File("lib1.xml"), parsed.libraries.get(0));
880+
assertEquals(new File("lib2.xml"), parsed.libraries.get(1));
881+
}
882+
883+
@Test
884+
public void testParseArgsPlatformIos() {
885+
ManifestMergeTool.ParsedArgs parsed = ManifestMergeTool.parseArgs(new String[]{"--platform", "ios"});
886+
assertEquals(Platform.IOS, parsed.platform);
887+
}
888+
889+
@Test
890+
public void testParseArgsPlatformOsx() {
891+
ManifestMergeTool.ParsedArgs parsed = ManifestMergeTool.parseArgs(new String[]{"--platform", "osx"});
892+
assertEquals(Platform.OSX, parsed.platform);
893+
}
894+
895+
@Test
896+
public void testParseArgsPlatformWeb() {
897+
ManifestMergeTool.ParsedArgs parsed = ManifestMergeTool.parseArgs(new String[]{"--platform", "web"});
898+
assertEquals(Platform.WEB, parsed.platform);
899+
}
900+
901+
@Test
902+
public void testParseArgsLibrariesOrderPreserved() {
903+
String[] args = {
904+
"--lib", "a.xml",
905+
"--lib", "b.xml",
906+
"--lib", "c.xml",
907+
};
908+
ManifestMergeTool.ParsedArgs parsed = ManifestMergeTool.parseArgs(args);
909+
assertEquals(3, parsed.libraries.size());
910+
assertEquals(new File("a.xml"), parsed.libraries.get(0));
911+
assertEquals(new File("b.xml"), parsed.libraries.get(1));
912+
assertEquals(new File("c.xml"), parsed.libraries.get(2));
913+
}
914+
915+
@Test
916+
public void testParseArgsUnsupportedPlatform() {
917+
IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
918+
() -> ManifestMergeTool.parseArgs(new String[]{"--platform", "windows"}));
919+
assertTrue(e.getMessage().contains("windows"));
920+
}
921+
922+
@Test
923+
public void testParseArgsUnknownFlag() {
924+
IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
925+
() -> ManifestMergeTool.parseArgs(new String[]{"--bogus", "value"}));
926+
assertTrue(e.getMessage().contains("--bogus"));
927+
}
928+
929+
@Test
930+
public void testParseArgsOddNumberOfArgs() {
931+
assertThrows(IllegalArgumentException.class,
932+
() -> ManifestMergeTool.parseArgs(new String[]{"--main"}));
933+
}
934+
935+
@Test
936+
public void testParseArgsOddNumberOfArgsTrailing() {
937+
assertThrows(IllegalArgumentException.class,
938+
() -> ManifestMergeTool.parseArgs(new String[]{"--main", "a.xml", "--lib"}));
939+
}
853940
}

‎server/src/main/java/com/defold/extender/ExtenderController.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -437,6 +437,6 @@ private RemoteInstanceConfig getRemoteBuilderConfig(String platform, String plat
437437
} else if (this.remoteBuilderPlatformMappings.containsKey(fallbackKey)) {
438438
return this.remoteBuilderPlatformMappings.get(fallbackKey);
439439
}
440-
throw new ExtenderException(String.format("No suitable remote builder found for %", fullKey));
440+
throw new ExtenderException(String.format("No suitable remote builder found for %s", fullKey));
441441
}
442442
}

‎server/src/main/java/com/defold/extender/process/ProcessExecutor.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public int execute(List<String> args) throws IOException, InterruptedException {
4646
if (DM_DEBUG_COMMANDS) {
4747
StringBuffer debugBuffer = new StringBuffer();
4848
debugBuffer.append(String.format("CMD %d: %s\n", commandId, String.join(" ", args)));
49-
debugBuffer.append(String.format("\tWorking dir: \n", this.cwd == null ? "(null)" : this.cwd.toString()));
49+
debugBuffer.append(String.format("\tWorking dir: %s\n", this.cwd == null ? "(null)" : this.cwd.toString()));
5050
debugBuffer.append("\tEnvironment:\n");
5151
for (Map.Entry<String, String> envEntry : this.env.entrySet()) {
5252
debugBuffer.append(String.format("\t%s=%s\n", envEntry.getKey(), envEntry.getValue()));

‎server/src/main/java/com/defold/extender/services/cocoapods/CocoaPodsService.java‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ private class InstalledPods {
6565
private static final Logger LOGGER = LoggerFactory.getLogger(CocoaPodsService.class);
6666
private static final String CURRENT_CACHE_DIR_FILE = "current_pod_cache.txt";
6767
private static final String OLD_CACHE_DIR_FILE = "old_pod_caches.txt";
68+
private final Object syncLock = new Object();
6869
private final TemplateExecutor templateExecutor = new TemplateExecutor();
6970

7071
private final String podfileTemplateContents;
@@ -88,13 +89,13 @@ public void runAfterStartup() {
8889
// initialize cache directory
8990
Path currentCacheDir = readCurrentCacheDir();
9091
if (currentCacheDir != null && currentCacheDir.startsWith(this.homeDirPrefix)) {
91-
synchronized(this.currentCacheDir) {
92+
synchronized(this.syncLock) {
9293
this.currentCacheDir = currentCacheDir;
9394
}
9495
updateSpecRepo();
9596
} else {
9697
LOGGER.info("Cocoapods has no current cache dir or prefix is changed. Created...");
97-
synchronized(this.currentCacheDir) {
98+
synchronized(this.syncLock) {
9899
this.currentCacheDir = generateCacheDirPath();
99100
storeCurrentCacheDir(this.currentCacheDir);
100101
}
@@ -216,7 +217,7 @@ private InstalledPods installPods(ExtenderBuildState buildState, CocoaPodsServic
216217
LOGGER.info("Installing pods");
217218
Path cacheDir;
218219
// store current cache dir into local variable to use the same value for all 'pod' runs
219-
synchronized(currentCacheDir) {
220+
synchronized(syncLock) {
220221
cacheDir = currentCacheDir;
221222
}
222223
InstalledPods installedPods = new InstalledPods();
@@ -501,7 +502,7 @@ private void storeCurrentCacheDir(Path currentCacheDir) {
501502
private void initializeTrunkRepo() {
502503
try {
503504
Path cacheDir;
504-
synchronized(currentCacheDir) {
505+
synchronized(syncLock) {
505506
cacheDir = currentCacheDir;
506507
}
507508
String log = ProcessUtils.execCommand(List.of(
@@ -524,7 +525,7 @@ public void rotatePodCacheDirectory() {
524525
LOGGER.info("Rotate pod cache directory");
525526
Path newCacheDir = generateCacheDirPath();
526527
Path cacheDir;
527-
synchronized(this.currentCacheDir) {
528+
synchronized(this.syncLock) {
528529
cacheDir = this.currentCacheDir;
529530
}
530531
try {
@@ -541,7 +542,7 @@ public void rotatePodCacheDirectory() {
541542
} catch(IOException exc) {
542543
LOGGER.warn("Error while writing to old cache paths file", exc);
543544
}
544-
synchronized(this.currentCacheDir) {
545+
synchronized(this.syncLock) {
545546
this.currentCacheDir = newCacheDir;
546547
storeCurrentCacheDir(currentCacheDir);
547548
}
@@ -578,7 +579,7 @@ public void updateSpecRepo() {
578579
try {
579580
LOGGER.info("Run pod spec update");
580581
Path cacheDir;
581-
synchronized(currentCacheDir) {
582+
synchronized(this.syncLock) {
582583
cacheDir = currentCacheDir;
583584
}
584585
String log = ProcessUtils.execCommand(List.of(

‎server/src/main/java/com/defold/extender/services/cocoapods/PodfileParser.java‎

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ public ParseResult mergeWith(ParseResult other) throws PodfileParsingException {
3131
minVersion = other.minVersion;
3232
}
3333

34+
3435
if (platform == null) {
3536
platform = other.platform;
3637
} else if (other.platform != null && !platform.equals(other.platform)) {
@@ -45,19 +46,40 @@ public ParseResult mergeWith(ParseResult other) throws PodfileParsingException {
4546
}
4647
}
4748

49+
// Strip semver pre-release and build metadata (everything from the first '-' or '+'),
50+
// e.g. "1.0.0-beta" -> "1.0.0", "2.0.0+build.7" -> "2.0.0". Comparison ignores these segments.
51+
private static String stripVersionSuffix(String version) {
52+
int cut = version.length();
53+
for (int i = 0; i < version.length(); i++) {
54+
char c = version.charAt(i);
55+
if (c == '-' || c == '+') {
56+
cut = i;
57+
break;
58+
}
59+
}
60+
return version.substring(0, cut);
61+
}
62+
4863
// https://www.baeldung.com/java-comparing-versions#customSolution
49-
static int compareVersions(String version1, String version2) {
64+
static int compareVersions(String version1, String version2) throws PodfileParsingException {
5065
int result = 0;
51-
String[] parts1 = version1.split("\\.");
52-
String[] parts2 = version2.split("\\.");
66+
String[] parts1 = stripVersionSuffix(version1).split("\\.");
67+
String[] parts2 = stripVersionSuffix(version2).split("\\.");
5368
int length = Math.max(parts1.length, parts2.length);
5469
for (int i = 0; i < length; i++) {
55-
Integer v1 = i < parts1.length ? Integer.parseInt(parts1[i]) : 0;
56-
Integer v2 = i < parts2.length ? Integer.parseInt(parts2[i]) : 0;
57-
int compare = v1.compareTo(v2);
58-
if (compare != 0) {
59-
result = compare;
60-
break;
70+
try {
71+
int v1 = i < parts1.length && !parts1[i].isEmpty() ? Integer.parseInt(parts1[i]) : 0;
72+
int v2 = i < parts2.length && !parts2[i].isEmpty() ? Integer.parseInt(parts2[i]) : 0;
73+
if (v1 < v2) {
74+
result = -1;
75+
break;
76+
} else if (v1 > v2) {
77+
result = 1;
78+
break;
79+
}
80+
} catch (NumberFormatException exc) {
81+
throw new PodfileParsingException(
82+
String.format("Failed to compare pod versions '%s' and '%s'", version1, version2), exc);
6183
}
6284
}
6385
return result;

‎server/src/main/java/com/defold/extender/services/cocoapods/PodfileParsingException.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,8 @@ public class PodfileParsingException extends ExtenderException {
66
public PodfileParsingException(String reason) {
77
super(reason);
88
}
9+
10+
public PodfileParsingException(String reason, Exception cause) {
11+
super(cause, reason);
12+
}
913
}

‎server/src/main/java/com/defold/extender/services/cocoapods/XCConfigParser.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,6 @@ Pair<String, String> parseLine(String line) {
144144
|| (c >= 'A' && c <= 'Z')
145145
|| c == '_') {
146146
varBuilder.append(c);
147-
} else if (c == '[') {
148-
currentMode = ParseMode.FLAVOUR_START;
149147
} else if (c == '=') {
150148
currentMode = ParseMode.ASSIGMENT_OPERATOR;
151149
} else {

0 commit comments

Comments
 (0)