Address PR comments, add case where input is negative, rebase

eclipse-vertx · Feb 20, 2024 · fe35bbd · fe35bbd
1 parent 7cd496b
commit fe35bbd
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 9 deletions.
diff --git a/src/main/java/io/vertx/core/http/impl/Http1xServerResponse.java b/src/main/java/io/vertx/core/http/impl/Http1xServerResponse.java
@@ -22,6 +22,7 @@
 import io.netty.handler.codec.http.HttpResponseStatus;
 import io.netty.handler.codec.http.HttpVersion;
 import io.netty.handler.codec.http.LastHttpContent;
+import io.netty.util.internal.ObjectUtil;
 import io.vertx.codegen.annotations.Nullable;
 import io.vertx.core.AsyncResult;
 import io.vertx.core.Future;
@@ -541,6 +542,8 @@ public HttpServerResponse bodyEndHandler(Handler<Void> handler) {
   }
 
   private void doSendFile(String filename, long offset, long length, Handler<AsyncResult<Void>> resultHandler) {
+    ObjectUtil.checkPositiveOrZero(offset, "offset");
+    ObjectUtil.checkPositiveOrZero(length, "length");
     synchronized (conn) {
       checkValid();
       if (headWritten) {

diff --git a/src/main/java/io/vertx/core/http/impl/Http2ServerResponse.java b/src/main/java/io/vertx/core/http/impl/Http2ServerResponse.java
@@ -19,6 +19,7 @@
 import io.netty.handler.codec.http.HttpStatusClass;
 import io.netty.handler.codec.http2.DefaultHttp2Headers;
 import io.netty.handler.codec.http2.Http2Headers;
+import io.netty.util.internal.ObjectUtil;
 import io.vertx.codegen.annotations.Nullable;
 import io.vertx.core.*;
 import io.vertx.core.buffer.Buffer;
@@ -602,6 +603,8 @@ public Future<Void> sendFile(String filename, long offset, long length) {
 
   @Override
   public HttpServerResponse sendFile(String filename, long offset, long length, Handler<AsyncResult<Void>> resultHandler) {
+    ObjectUtil.checkPositiveOrZero(offset, "offset");
+    ObjectUtil.checkPositiveOrZero(length, "length");
     synchronized (conn) {
       checkValid();
     }

diff --git a/src/test/java/io/vertx/core/http/HttpTest.java b/src/test/java/io/vertx/core/http/HttpTest.java
@@ -2168,11 +2168,9 @@ public void testSendRangeFileFromClasspath() {
   }
 
   @Test
-  public void testSendZeroRangeFileFromClasspath() {
-    server.requestHandler(res -> {
-      // hosts_config.txt is 23 bytes
-      res.response().sendFile("hosts_config.txt", 23, 0);
-    }).listen(testAddress, onSuccess(res -> {
+  public void testSendZeroRangeFile() throws Exception {
+    File f = setupFile("twenty_three_bytes.txt", TestUtils.randomAlphaString(23));
+    server.requestHandler(res -> res.response().sendFile(f.getAbsolutePath(), 23, 0)).listen(testAddress, onSuccess(res -> {
       client.request(requestOptions)
         .compose(HttpClientRequest::send)
         .compose(resp -> {
@@ -2189,11 +2187,11 @@ public void testSendZeroRangeFileFromClasspath() {
   }
 
   @Test
-  public void testSendOffsetIsHigherThanFileLengthForFileFromClasspath() {
+  public void testSendOffsetIsHigherThanFileLengthForFile() throws Exception {
+    File f = setupFile("twenty_three_bytes.txt", TestUtils.randomAlphaString(23));
     server.requestHandler(res -> {
       try {
-        // hosts_config.txt is 23 bytes
-        res.response().sendFile("hosts_config.txt", 33, 10)
+        res.response().sendFile(f.getAbsolutePath(), 33, 10)
           .onFailure(throwable -> {
             try {
               res.response().setStatusCode(500).end();
@@ -2225,6 +2223,61 @@ public void testSendOffsetIsHigherThanFileLengthForFileFromClasspath() {
     await();
   }
 
+  @Test
+  public void testSendFileFromClasspathWithNegativeLength() throws Exception {
+    File f = setupFile("twenty_three_bytes.txt", TestUtils.randomAlphaString(23));
+    server.requestHandler(res -> {
+      try {
+        res.response().sendFile(f.getAbsolutePath(), 0, -100)
+          .onFailure(throwable -> {
+            // should not reach here, the response should not be sent if the offset is negative
+            fail("Should not reach here");
+          });
+      } catch (Exception ex) {
+        assertEquals(IllegalArgumentException.class, ex.getClass());
+        assertEquals("length : -100 (expected: >= 0)", ex.getMessage());
+        // handle failure in sendFile
+        res.response().setStatusCode(500).end();
+      }
+    });
+    startServer(testAddress);
+    client.request(requestOptions)
+      .compose(HttpClientRequest::send)
+      .onComplete(onSuccess(response -> {
+        assertEquals(500, response.statusCode());
+        testComplete();
+      }));
+
+    await();
+  }
+
+  @Test
+  public void testSendFileFromClasspathWithNegativeOffset() throws Exception {
+    File f = setupFile("twenty_three_bytes.txt", TestUtils.randomAlphaString(23));
+    server.requestHandler(res -> {
+      try {
+        res.response().sendFile(f.getAbsolutePath(), -100, 23)
+          .onFailure(throwable -> {
+            // should not reach here, the response should not be sent if the offset is negative
+            fail("Should not reach here");
+          });
+      } catch (Exception ex) {
+        assertEquals(IllegalArgumentException.class, ex.getClass());
+        assertEquals("offset : -100 (expected: >= 0)", ex.getMessage());
+        res.response().setStatusCode(500).end();
+      }
+    });
+    startServer(testAddress);
+    client.request(requestOptions)
+      .compose(HttpClientRequest::send)
+      .onComplete(onSuccess(response -> {
+        assertEquals(500, response.statusCode());
+        testComplete();
+      }));
+
+    await();
+  }
+
   @Test
   public void test100ContinueHandledAutomatically() {
     Buffer toSend = TestUtils.randomBuffer(1000);
@@ -5000,7 +5053,7 @@ protected void testKeepAliveTimeout(HttpClientOptions options, int numReqs) thro
     client.close();
     client = vertx.createHttpClient(options.setPoolCleanerPeriod(1));
     AtomicInteger respCount = new AtomicInteger();
-    for (int i = 0;i < numReqs;i++) {
+    for (int i = 0; i < numReqs; i++) {
       int current = 1 + i;
       client.request(requestOptions)
         .compose(HttpClientRequest::send)