Skip to content

Commit 81bda76

Browse files
Copilotithewei
andcommitted
Address code review: add fd validation, fix partial write handling, improve error messages
- Add in_fd validation in hio_sendfile() for both nio.c and overlapio.c - Fix generic sendfile fallback to handle partial writes correctly - Check sendfile_remain under mutex before unlocking in nio_write - Improve SSL fallback with better EOF/error distinction - Add fileno() validation in HTTP handler - Improve Windows fallback with separate seek/read error handling Co-authored-by: ithewei <26049660+ithewei@users.noreply.github.com> Agent-Logs-Url: https://github.com/ithewei/libhv/sessions/e59504ed-e745-410d-a8ab-e99946d80a3c
1 parent bbbd87a commit 81bda76

File tree

3 files changed

+41
-15
lines changed

3 files changed

+41
-15
lines changed

event/nio.c

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -334,16 +334,26 @@ static ssize_t __nio_sendfile_sys(int out_fd, int in_fd, off_t* offset, size_t c
334334
}
335335
return -1;
336336
#else
337-
// Fallback: read + write
337+
// Fallback: pread + write (sends one chunk per call to integrate with event loop)
338338
char buf[65536];
339339
size_t to_read = count < sizeof(buf) ? count : sizeof(buf);
340340
ssize_t nread = pread(in_fd, buf, to_read, *offset);
341341
if (nread <= 0) return nread;
342-
ssize_t nwrite = write(out_fd, buf, nread);
343-
if (nwrite > 0) {
344-
*offset += nwrite;
342+
ssize_t total_written = 0;
343+
while (total_written < nread) {
344+
ssize_t nwrite = write(out_fd, buf + total_written, nread - total_written);
345+
if (nwrite < 0) {
346+
if (total_written > 0) {
347+
*offset += total_written;
348+
return total_written;
349+
}
350+
return -1;
351+
}
352+
if (nwrite == 0) break;
353+
total_written += nwrite;
345354
}
346-
return nwrite;
355+
*offset += total_written;
356+
return total_written;
347357
#endif
348358
}
349359

@@ -480,11 +490,12 @@ static void nio_write(hio_t* io) {
480490
if (nsent == 0 && (io->io_type & HIO_TYPE_SOCK_STREAM)) {
481491
goto disconnect;
482492
}
493+
int complete = (io->sendfile_remain == 0);
483494
hrecursive_mutex_unlock(&io->write_mutex);
484495
if (nsent > 0) {
485496
__write_cb(io, NULL, nsent);
486497
}
487-
if (io->sendfile_remain == 0) {
498+
if (complete) {
488499
io->sendfile_fd = -1;
489500
}
490501
return;
@@ -679,20 +690,30 @@ int hio_sendfile (hio_t* io, int in_fd, off_t offset, size_t length) {
679690
hloge("hio_sendfile called but fd[%d] already closed!", io->fd);
680691
return -1;
681692
}
693+
if (in_fd < 0) {
694+
hloge("hio_sendfile invalid file descriptor: %d", in_fd);
695+
return -1;
696+
}
682697
if (length == 0) return 0;
683698

684699
// SSL: fall back to read + hio_write (sendfile cannot bypass SSL encryption)
700+
// NOTE: hio_write is non-blocking and queues data internally,
701+
// so this won't block the event loop even for large files.
685702
if (io->io_type == HIO_TYPE_SSL) {
686703
char buf[65536];
687704
off_t cur_offset = offset;
688705
size_t remaining = length;
689706
while (remaining > 0) {
690707
size_t to_read = remaining < sizeof(buf) ? remaining : sizeof(buf);
691708
ssize_t nread = pread(in_fd, buf, to_read, cur_offset);
692-
if (nread <= 0) {
693-
hloge("hio_sendfile pread error!");
709+
if (nread < 0) {
710+
hloge("hio_sendfile pread error: %s", strerror(errno));
694711
return -1;
695712
}
713+
if (nread == 0) {
714+
hlogw("hio_sendfile: unexpected EOF at offset %lld", (long long)cur_offset);
715+
break;
716+
}
696717
int nwrite = hio_write(io, buf, nread);
697718
if (nwrite < 0) return nwrite;
698719
cur_offset += nread;
@@ -721,9 +742,10 @@ int hio_sendfile (hio_t* io, int in_fd, off_t offset, size_t length) {
721742
}
722743
}
723744
if (nsent > 0) {
745+
int complete = (io->sendfile_remain == 0);
724746
hrecursive_mutex_unlock(&io->write_mutex);
725747
__write_cb(io, NULL, nsent);
726-
if (io->sendfile_remain == 0) {
748+
if (complete) {
727749
io->sendfile_fd = -1;
728750
return 0;
729751
}

event/overlapio.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -389,15 +389,19 @@ int hio_sendto (hio_t* io, const void* buf, size_t len, struct sockaddr* addr) {
389389

390390
int hio_sendfile (hio_t* io, int in_fd, off_t offset, size_t length) {
391391
if (io->closed) return -1;
392+
if (in_fd < 0) return -1;
392393
if (length == 0) return 0;
393-
// Fallback: read + hio_write
394+
// NOTE: Windows fallback uses read + hio_write.
395+
// hio_write is non-blocking (queues via IOCP), so this won't block.
394396
char buf[65536];
395397
off_t cur_offset = offset;
396398
size_t remaining = length;
397399
while (remaining > 0) {
398400
size_t to_read = remaining < sizeof(buf) ? remaining : sizeof(buf);
399-
ssize_t nread = _lseeki64(in_fd, cur_offset, SEEK_SET) >= 0 ? _read(in_fd, buf, (unsigned int)to_read) : -1;
400-
if (nread <= 0) return -1;
401+
if (_lseeki64(in_fd, cur_offset, SEEK_SET) < 0) return -1;
402+
ssize_t nread = _read(in_fd, buf, (unsigned int)to_read);
403+
if (nread < 0) return -1;
404+
if (nread == 0) break; // EOF
401405
int nwrite = hio_write(io, buf, nread);
402406
if (nwrite < 0) return nwrite;
403407
cur_offset += nread;

http/server/HttpHandler.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -659,8 +659,8 @@ int HttpHandler::defaultLargeFileHandler(const std::string &filepath) {
659659
// forbidden to send large file
660660
resp->content_length = 0;
661661
resp->status_code = HTTP_STATUS_FORBIDDEN;
662-
} else if (service->limit_rate < 0 && file->fp) {
663-
// unlimited: use zero-copy sendfile if possible
662+
} else if (service->limit_rate < 0 && file->fp && fileno(file->fp) >= 0) {
663+
// unlimited: use zero-copy sendfile
664664
int filefd = fileno(file->fp);
665665
size_t length = resp->content_length;
666666
writer->EndHeaders();
@@ -677,7 +677,7 @@ int HttpHandler::defaultLargeFileHandler(const std::string &filepath) {
677677
size_t bufsize = 40960; // 40K
678678
file->buf.resize(bufsize);
679679
if (service->limit_rate < 0) {
680-
// unlimited: sendFile when writable (fallback when fileno unavailable)
680+
// unlimited: sendFile when writable
681681
writer->onwrite = [this](HBuf* buf) {
682682
if (writer->isWriteComplete()) {
683683
sendFile();

0 commit comments

Comments
 (0)