Skip to content

Conversation

@gvanem
Copy link

@gvanem gvanem commented May 17, 2021

A highsize should be the upper 32-bit of a signed 64-bit value.
But nobody have ever tried this function with a >2GByte .log-file before (?)

A `highsize` should be the upper 32-bit of a signed 64-bit value.
But nobody have ever tried this function with a >2GByte .log-file before (?)
@nnposter
Copy link

I am in agreement that the current arithmetic is unsafe and that your code would fix it. That said, I wonder whether it should not be addressed in a more modern way, which avoids the arithmetic altogether, such as:

--- a/utils.cc
+++ b/utils.cc
@@ -595,7 +595,7 @@
 char *mmapfile(char *fname, s64 *length, int openflags) {
   HANDLE fd;
   DWORD mflags, oflags;
-  DWORD lowsize, highsize;
+  LARGE_INTEGER filesize;
   char *fileptr;
 
   if (!length || !fname) {
@@ -622,11 +622,10 @@
   if (!fd)
     pfatal ("%s(%u): CreateFile()", __FILE__, __LINE__);
 
-  lowsize = GetFileSize (fd, &highsize);
-  if (lowsize == INVALID_FILE_SIZE && GetLastError() != NO_ERROR) {
-    pfatal("%s(%u): GetFileSize(), file '%s'", __FILE__, __LINE__, fname);
+  if (!GetFileSizeEx(fd, &filesize)) {
+    pfatal("%s(%u): GetFileSizeEx(), file '%s'", __FILE__, __LINE__, fname);
   }
-  *length = lowsize + highsize << sizeof(DWORD);
+  *length = (s64)filesize.QuadPart;
   if (*length < 0) {
     fatal("%s(%u): size too large, file '%s'", __FILE__, __LINE__, fname);
   }

Thoughts?

@gvanem
Copy link
Author

gvanem commented May 17, 2021

Thoughts?

A GetFileSizeEx() is fine with me.

@nnposter
Copy link

Committed as r38219.
Thank you for identifying the issue and proposing a fix.

@nnposter nnposter self-assigned this May 18, 2021
@nmap-bot nmap-bot closed this in c3d9d16 May 18, 2021
mzet- pushed a commit to mzet-/Nmap-for-Pen-Testers that referenced this pull request Dec 20, 2021
The old code was incorrectly calculating sizes of files exceeding 4 GB.
The new code skips the arithmetic altogether by using a different API.
@nmap nmap deleted a comment Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants