heap-buffer-overflow when using io-related functions

[Osyotr]

Actual behavior

Writing images and views to files does not work because string conversion functions are not implemented properly:

gil/include/boost/gil/io/path_spec.hpp

Lines 90 to 97 in eabd679

	inline char const* convert_to_native_string( std::wstring const& str )
	{
	std::size_t len = wcslen( str.c_str() ) + 1;
	char* c = new char[len];
	wcstombs( c, str.c_str(), len );
	return c;
	}

The code above does not work for paths that contain non-ascii symbols. The string it produces does not have \0 at the end.

Expected behavior

Passing std::filesystem::path or std::wstring to io-related functions should work properly.
Possible solution is to remove explicit string conversions, construct std::filesystem::path and use its .string() method.
Note that it may break on windows because of usage of fopen

gil/include/boost/gil/io/device.hpp

Line 105 in eabd679

io_error_if( ( file = fopen( file_name, "wb" )) == nullptr

(_wfopen should be used on windows)

C++ Minimal Working Example

I've extracted broken part (linked above) to reproduce the issue: https://godbolt.org/z/rvxsPG7a4

#include <boost/gil.hpp>
#include <filesystem>
namespace gil = boost::gil;
int main
{
    std::filesystem::path path = L"/some_path/傳/привет/qwerty";
    gil::bgra8_image_t image;
    gil::write_view(path, gil::const_view(image));
}

Environment

• Compiler version: GCC 11.2
• Build settings: None
• Version: 1.80

[striezel]

Yes, this is a bug.

The length calculation is wrong. The code uses wcslen() to "calculate" the length of the converted string, however wcslen() is just a strlen() for wchar_t* strings. So it returns the number of wide characters in a wchar_t* string, but those do not need to be the same as the number of narrow characters (that is: char) in the converted string. It just happens to be the same, if the wide character string only uses characters of the English alphabet where each wide character is converted to exactly one narrow character.

Instead of wcslen() the code could use wcstombs()'s POSIX extension to calculate the length before doing the conversion.

POSIX specifies a common extension: if dst is a null pointer, this function returns the number of bytes that would be written to dst, if converted.

But that is an extension, so we should probably use wcsrtombs() instead, where that behaviour is not an extension but part of the function and we do not need to rely on the presence of an extension.

Long story short: I'll try to prepare a fix.