Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OOM when load_file for special folder #560

Closed
TERESH1 opened this issue Apr 11, 2023 · 5 comments · Fixed by #561
Closed

OOM when load_file for special folder #560

TERESH1 opened this issue Apr 11, 2023 · 5 comments · Fixed by #561
Labels

Comments

@TERESH1
Copy link

TERESH1 commented Apr 11, 2023

Sometimes when you use load_file with ASAN for special folder it can cause OOM exception:

root@fuzzing:/home/user/pugixml/src# cat main.cpp
#include "pugixml.hpp"
int main(int argc, char **argv) {
   pugi::xml_document doc;
   doc.load_file(argv[1], 116U, pugi::encoding_auto);
   return 0;
}
root@fuzzing:/home/user/pugixml/src# clang++ -fsanitize=address -g -O0 -ferror-limit=1 main.cpp pugixml.cpp -o main_asan
root@fuzzing:/home/user/pugixml/src# ./main_asan /
root@fuzzing:/home/user/pugixml/src# ./main_asan /home/
root@fuzzing:/home/user/pugixml/src# ./main_asan /home/user/
root@fuzzing:/home/user/pugixml/src# ./main_asan /home/user/pugixml/
=================================================================
==269==ERROR: AddressSanitizer: requested allocation size 0x8000000000000000 (0x8000000000001000 after adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x10000000000 (thread T0)
    #0 0x5557c32944be in malloc (/home/user/pugixml/src/main_asan+0xad4be) (BuildId: 20f3d37b214a5fd4245eefc0288fa6b0563186d6)
    #1 0x5557c334c6e4 in pugi::impl::(anonymous namespace)::default_allocate(unsigned long) /home/user/pugixml/src/pugixml.cpp:190:10
    #2 0x5557c32ec0df in pugi::impl::(anonymous namespace)::load_file_impl(pugi::impl::(anonymous namespace)::xml_document_struct*, _IO_FILE*, unsigned int, pugi::xml_encoding, char**) /home/user/pugixml/src/pugixml.cpp:4836:39
    #3 0x5557c32ebd36 in pugi::xml_document::load_file(char const*, unsigned int, pugi::xml_encoding) /home/user/pugixml/src/pugixml.cpp:7347:10
    #4 0x5557c32d1a31 in main /home/user/pugixml/src/main.cpp:4:8
    #5 0x7f7611ba2d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)

==269==HINT: if you don't care about these errors you may set allocator_may_return_null=1
SUMMARY: AddressSanitizer: allocation-size-too-big (/home/user/pugixml/src/main_asan+0xad4be) (BuildId: 20f3d37b214a5fd4245eefc0288fa6b0563186d6) in malloc
==269==ABORTING
root@fuzzing:/home/user/pugixml/src# cat /etc/*release*
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=22.04
DISTRIB_CODENAME=jammy
DISTRIB_DESCRIPTION="Ubuntu 22.04.1 LTS"
PRETTY_NAME="Ubuntu 22.04.1 LTS"
NAME="Ubuntu"
VERSION_ID="22.04"
VERSION="22.04.1 LTS (Jammy Jellyfish)"
VERSION_CODENAME=jammy
ID=ubuntu
ID_LIKE=debian
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
UBUNTU_CODENAME=jammy
root@fuzzing:/home/user/pugixml/src# clang --version
Ubuntu clang version 14.0.6
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
root@fuzzing:/home/user/pugixml/src#

In such cases on Linux fseek return 0 and ftell return INT64_MAX.

You should check if a file is a regular file.

@TERESH1 TERESH1 added the bug label Apr 11, 2023
@TERESH1
Copy link
Author

TERESH1 commented Apr 11, 2023

For checking it and getting file size you can use something like this:

#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>

int get_file_size(const char* filename, size_t& out_result) {
   struct stat st;
   if(stat(filename, &st) != 0) {
      // return error
   }
   if (!S_ISREG(st.st_mode)) {
      // return error
   }
   out_result = st.st_size;
   // return ok
}

Or use Filesystem library (since C++17) for getting size: std::filesystem::path with std::filesystem::file_size

In case of using it with dirs it throws exception:

terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
  what():  filesystem error: cannot get file size: Is a directory [/]

@zeux
Copy link
Owner

zeux commented Apr 15, 2023

Thanks! This should not have production impact, as the code correctly handles the OOM, but is certainly inconvenient for fuzzing. ftell here returns the maximum signed integer of the relevant type instead of returning a negative number for some reason.

std::filesystem can't be used in pugixml as it doesn't use STL except for STL-specific API helpers that can be disabled, and carries compatibility burden on systems like macOS. It is probably better to solve this by explicitly checking the size; stat could work but has some other issues and an extra syscall per file open is not optimal for some workloads. That said, maybe stat is actually fine; I'll look into this further.

@zeux
Copy link
Owner

zeux commented Apr 15, 2023

Ah, looks like I hit this before :) 7664bbf

@zeux
Copy link
Owner

zeux commented Apr 15, 2023

Looks like fseek/ftell use fstatat under the hood, so stat is actually more efficient (only matters for small files, but right now it takes us 7 syscalls to read a file, and with stat it only takes 5; one of these is done by fread and can be removed by switching to open/read or by disabling buffering via setvbuf).

@TERESH1
Copy link
Author

TERESH1 commented Apr 17, 2023

The bug was found with Futag

@zeux zeux closed this as completed in #561 Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants