Skip to content

Fix seg fault when using iconv_open() #2403

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 1, 2022
Merged

Fix seg fault when using iconv_open() #2403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d41582c
Fix seg fault when using `iconv_open()`
postscript-dev Nov 1, 2022
37d6249
Add testing for `iconv_open()` seg fault bug
postscript-dev Nov 1, 2022
7d69936
Fix Python test by changing log level
postscript-dev Nov 1, 2022
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/exiv2/error.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ enum class ErrorCode {
kerCorruptedMetadata,
kerArithmeticOverflow,
kerMallocFailed,
kerInvalidIconvEncoding,

kerErrorCount,
};
Expand Down
10 changes: 8 additions & 2 deletions src/convert.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,13 @@ void Converter::cnvExifComment(const char* from, const char* to) {
return;
}
// Todo: Convert to UTF-8 if necessary
(*xmpData_)[to] = cv->comment();
try {
(*xmpData_)[to] = cv->comment();
} catch (const Error&) {
#ifndef SUPPRESS_WARNINGS
EXV_WARNING << "Failed to convert " << from << " to " << to << "\n";
#endif
}
if (erase_)
exifData_->erase(pos);
}
Expand Down Expand Up @@ -1567,7 +1573,7 @@ bool convertStringCharsetIconv(std::string& str, const char* from, const char* t
bool ret = true;
iconv_t cd;
cd = iconv_open(to, from);
if (!cd) {
if (cd == (iconv_t)(-1)) {
Copy link
Collaborator

@piponazo piponazo Nov 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] could you define and assign the cd in the same line? Furthermore, is it really needed the cast to iconv_t if we already know that the variable cd is of that type?
Copy link
Collaborator

@neheb neheb Jan 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

musl libc (and most others) have it as
typedef void *iconv_t;

uclibc-ng has it as
typedef long iconv_t;

cast is needed if it's a pointer I think.

This does trigger a clang-tidy check that it's a C cast.

I wonder if there's a C++ way to do this...
Copy link
Collaborator Author

@postscript-dev postscript-dev Jan 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neheb

musl libc (and most others) have it as
typedef void *iconv_t;

uclibc-ng has it as
typedef long iconv_t;

Thanks for letting me know, I wasn't aware of the different definitions. As for a C++ solution, would using std::is_pointer_v help?
Copy link
Collaborator

@neheb neheb Jan 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No i mean you can't use static_cast or reinterpret_cast as it would fail with void* and long respectively.

if std::is_pointer_v<cd> ? reinterpret_cast<iconv_t>(-1) : static_cast<iconv_t>(-1))

is quite verbose.

if (iconv_t(-1))

is an alternative but newer clang-tidy warns on that as well.
#ifndef SUPPRESS_WARNINGS
EXV_WARNING << "iconv_open: " << strError() << "\n";
#endif
Expand Down
1 change: 1 addition & 0 deletions src/error.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ constexpr std::array errList{
N_("corrupted image metadata"), // kerCorruptedMetadata
N_("Arithmetic operation overflow"), // kerArithmeticOverflow
N_("Memory allocation failed"), // kerMallocFailed
N_("Cannot convert text encoding from '%1' to '%2'"), // kerInvalidIconvEncoding
};
static_assert(errList.size() == static_cast<size_t>(Exiv2::ErrorCode::kerErrorCount),
"errList needs to contain a error msg for every ErrorCode defined in error.hpp");
Expand Down
6 changes: 4 additions & 2 deletions src/value.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,10 +370,11 @@ size_t CommentValue::copy(byte* buf, ByteOrder byteOrder) const {

std::ostream& CommentValue::write(std::ostream& os) const {
CharsetId csId = charsetId();
std::string text = comment();
if (csId != undefined) {
os << "charset=" << CharsetInfo::name(csId) << " ";
}
return os << comment();
return os << text;
}

std::string CommentValue::comment(const char* encoding) const {
Expand All @@ -384,7 +385,8 @@ std::string CommentValue::comment(const char* encoding) const {
c = value_.substr(8);
if (charsetId() == unicode) {
const char* from = !encoding || *encoding == '\0' ? detectCharset(c) : encoding;
convertStringCharset(c, from, "UTF-8");
if (!convertStringCharset(c, from, "UTF-8"))
throw Error(ErrorCode::kerInvalidIconvEncoding, encoding, "UTF-8");
}
bool bAscii = charsetId() == undefined || charsetId() == ascii;
// # 1266 Remove trailing nulls
Expand Down
Binary file added test/data/issue_2403_poc.exv
View file
Open in desktop
Binary file not shown.
25 changes: 25 additions & 0 deletions tests/bugfixes/github/test_issue_2403.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# -*- coding: utf-8 -*-

from system_tests import CaseMeta, CopyTmpFiles, check_no_ASAN_UBSAN_errors
@CopyTmpFiles("$data_path/issue_2403_poc.exv")

class checkIconvSegFault(metaclass=CaseMeta):
url = """"""
description = """Test the fixcom action in the exiv2 app"""

filename = """$tmp_path/issue_2403_poc.exv"""

commands = ["""$exiv2 --verbose --log e --encode made_up_encoding fixcom $filename""",
"""$exiv2 --verbose --keep --encode UCS-2LE fixcom $filename"""]
retval = [1,0]

stdout = ["""File 1/1: $filename
""",
"""File 1/1: $filename
Setting Exif UNICODE user comment to "Test"
"""]

stderr = ["""Exiv2 exception in fixcom action for file $filename:
Cannot convert text encoding from 'made_up_encoding' to 'UTF-8'
""",
""""""]
1 change: 1 addition & 0 deletions tests/regression_tests/test_regression_allfiles.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ def get_valid_files(data_dir):
# this test file actually contains some eixf info, but windows has
# different output let's try and fix this later
"exiv2-bug1044.tif",
"issue_2403_poc.exv",
]

file_paths = [
Expand Down