Re: [PR] SOLR-16341: fix blank file zip handling [solr]
epugh merged PR #4249: URL: https://github.com/apache/solr/pull/4249 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-16341: fix blank file zip handling [solr]
janhoy commented on code in PR #4249:
URL: https://github.com/apache/solr/pull/4249#discussion_r3279486385
##
solr/core/src/java/org/apache/solr/handler/configsets/UploadConfigSet.java:
##
@@ -85,22 +89,41 @@ public SolrJerseyResponse uploadConfigSet(
filesToDelete = new ArrayList<>();
}
-try (ZipInputStream zis = new ZipInputStream(requestBody,
StandardCharsets.UTF_8)) {
- boolean hasEntry = false;
- ZipEntry zipEntry;
- while ((zipEntry = zis.getNextEntry()) != null) {
-hasEntry = true;
-String filePath = zipEntry.getName();
-filesToDelete.remove(filePath);
-if (!zipEntry.isDirectory()) {
- configSetService.uploadFileToConfig(configSetName, filePath,
zis.readAllBytes(), true);
+// Write the request body to a temp file so we can use ZipFile, which
reads the central
+// directory and correctly handles entries that use the STORED method with
an EXT (data
+// descriptor) flag — a combination that ZipInputStream cannot process.
This allows
+// zero-byte files (e.g. created with `touch`) to be included in the
uploaded configset.
+final Path tempZip = Files.createTempFile("solr-configset-upload-",
".zip");
+try {
+ Files.copy(requestBody, tempZip, StandardCopyOption.REPLACE_EXISTING);
Review Comment:
Do we need a guard against huge streams here, e.g. 2Gb, or is that handled
elsewhere on the network layer?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-16341: fix blank file zip handling [solr]
epugh commented on PR #4249: URL: https://github.com/apache/solr/pull/4249#issuecomment-4320411032 I don't love how verbose the test is for constructing the bad zip file. I played around with some other ways of generating it, but none looked good. So, since it's just in a test, and has comments, I'll probably just merge this. Speak up if you hate the test ;-) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-16341: fix blank file zip handling [solr]
epugh commented on PR #4249: URL: https://github.com/apache/solr/pull/4249#issuecomment-4162075820 @copilot is there a way to properly handle the zero byte file? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-16341: fix blank file zip handling [solr]
epugh commented on PR #4249: URL: https://github.com/apache/solr/pull/4249#issuecomment-4162071412 I will probably merge this, but I don't love it. A lot of code for a werid situation that I wish just got handled by Java instead! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-16341: fix blank file zip handling [solr]
Copilot commented on code in PR #4249:
URL: https://github.com/apache/solr/pull/4249#discussion_r3006689407
##
solr/core/src/java/org/apache/solr/handler/configsets/UploadConfigSet.java:
##
@@ -80,13 +81,22 @@ public SolrJerseyResponse uploadConfigSet(
try (ZipInputStream zis = new ZipInputStream(requestBody,
StandardCharsets.UTF_8)) {
boolean hasEntry = false;
ZipEntry zipEntry;
- while ((zipEntry = zis.getNextEntry()) != null) {
-hasEntry = true;
-String filePath = zipEntry.getName();
-filesToDelete.remove(filePath);
-if (!zipEntry.isDirectory()) {
- configSetService.uploadFileToConfig(configSetName, filePath,
zis.readAllBytes(), true);
+ try {
+while ((zipEntry = zis.getNextEntry()) != null) {
+ hasEntry = true;
+ String filePath = zipEntry.getName();
+ filesToDelete.remove(filePath);
+ if (!zipEntry.isDirectory()) {
+configSetService.uploadFileToConfig(configSetName, filePath,
zis.readAllBytes(), true);
+ }
}
+ } catch (ZipException e) {
+throw new SolrException(
+SolrException.ErrorCode.BAD_REQUEST,
+"Failed to read the uploaded zip file. The file may be malformed
or use an unsupported format. "
++ "Please recreate the zip file using standard compression
tools: "
++ e.getMessage(),
+e);
Review Comment:
The BAD_REQUEST message here advises recreating the zip using “standard
compression tools”, but the new test’s javadoc notes that common tooling (e.g.,
`zip -r` with an empty file) can legitimately produce this
STORED+data-descriptor variant. Consider rewording the message to explain the
specific unsupported ZIP feature (STORED entry with data descriptor / EXT flag)
and suggest actionable mitigations (e.g., use DEFLATED compression or avoid
0-byte STORED entries with descriptors), rather than implying the zip was
created with non-standard tools. Also, appending `e.getMessage()`
unconditionally can yield a trailing "null"; guard against null or format it
conditionally.
```suggestion
StringBuilder msg =
new StringBuilder(
"Failed to read the uploaded zip file. The file may be
malformed or use an unsupported ZIP feature (for example, a STORED entry with a
data descriptor / EXT flag). "
+ "Try recreating the zip using DEFLATED compression or
avoid zero-byte STORED entries with data descriptors.");
String underlying = e.getMessage();
if (underlying != null && !underlying.isEmpty()) {
msg.append(" Underlying error: ").append(underlying);
}
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
msg.toString(), e);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
