Handle failed iconv conversion. Unhandled execution path was UB, causing a segfault for me#5329
Conversation
…ing a segfault for me FontForge does some locale conversions before printing errors. NOUI_IError and NOUI__LogError (in nouiutil.c) calls utf82def_copy (in ucharmap.c), which calls do_iconv. do_iconv handles failed iconv call and returns NULL, which propagates back to NOUI_IError and NOUI__LogError functions and is not handled there. NOUI_IError and NOUI__LogError are calling printf with NULL as an argument to "%s", which is undefined behaviour and that UB is crashing my Android devices. Yes, I do understand, that I should fix the iconv call to not fail, but until then, I wouldn't mind fixing this UB, if that looks acceptable.
|
This looks good to me, but could you rebase @ViliusSutkus89 ? I don't want to override the checks on all of the back-log. |
| putc('\n',stderr); | ||
| free(str); | ||
| } else { | ||
| fprintf(stderr, "utf82def_copy failure!\n"); |
There was a problem hiding this comment.
Throwing the error out because iconv() has opinions doesn't seem right to me. Maybe add fprintf(stderr,"%s",buffer); here, just to print whatever we have, even if badly encoded?
There was a problem hiding this comment.
Only issue I could see would be malformed characters being interpreted as terminal control characters
There was a problem hiding this comment.
@ViliusSutkus89, do you have a sample font + steps to reproduce this?
There was a problem hiding this comment.
I have a broken iconv that's causing the issue :D But it is still UB.
The original error message is "The PostScript font name "BGDGBW弫Roboto-Bold" is invalid.\nIt should be printable ASCII,\nmust not contain (){}[]<>%/ or space\nand must be shorter than 63 characters"
There was a problem hiding this comment.
Broken iconv would mean that any error message would trigger UB here
There was a problem hiding this comment.
I understand that we use iconv() mainly for two things: file names and error messages to the terminal. Maybe we can add separate iconv_t descriptors for errors using ASCII//TRANSLIT as a fallback? In that case your error message would become "The PostScript font name "BGDGBW?Roboto-Bold...", which is still informative.
|
Related issue: #5326 |
|
This patch fixes #5326 (which calls @ViliusSutkus89, could you please apply and verify with your font? |
|
@iorsh , yes! this works for me. I still get "utf82def_copy failure!", but that's because of my broken iconv |
@ViliusSutkus89 Great, could you please commit the patch into your PR? I don't have access. |
|
Sure thing, @iorsh . I've also pulled commits from current master branch, for a cleaner merge. |
|
Sweet, thanks :) |
Type of change
Hello,
FontForge does some locale conversions before printing errors.
NOUI_IErrorandNOUI__LogError(innouiutil.c) callsutf82def_copy(inucharmap.c), which callsdo_iconv.do_iconvhandles failediconvcall and returnsNULL, which propagates back toNOUI_IErrorandNOUI__LogErrorfunctions and is not handled there.NOUI_IErrorandNOUI__LogErrorare callingprintfwithNULLas an argument to"%s", which is undefined behaviour and that UB is crashing my Android devices.Yes, I do understand, that I should fix the
iconvcall to not fail, but until then, I wouldn't mind fixing this UB, if that looks acceptable.Regards,
Vilius