Makes TChDirDialog on Linux handle native Linux paths (and fix crash)

[MookThompson]

Hi magliblot,

For the record, here are the hacky changes I made to get the TChDirDialog to work on Linux with native paths (and not crash when doing so). For my use, I don't need/want the Linux TChDirDialog to handle Windows paths, so it doesn't (eg it should accept names with the valid-on-Linux '\' char in it without assuming that's a directory separator). Because I don't need to support Windows paths on Linux, it's also switched based on the existing _TV_UNIX #define, rather than some runtime flag.

The fix for the crash is the "if ( end == NULL )" check in source/tvision/tdirlist.cpp.

As discussed, there could well be other places where the 'Windows paths on Linux' is still assumed - I've just done what I needed to get the TChDirDialog up and running as I wanted it.

Hope this helps you do it properly if you get chance to look at it at some point.
Mark

[jengelh]

This does not fix the crash for me. ASAN still reports an issue.

=================================================================
==64147==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x507000000a74 at pc 0x7f228aca97a1 bp 0x7ffc9a2996a0 sp 0x7ffc9a298e60
WRITE of size 120 at 0x507000000a74 thread T0
    #0 0x7f228aca97a0  (/lib64/libasan.so.8+0xa97a0) (BuildId: 183f53e480d86b3f4e532dc084c6a3c77ec09062)
    #1 0x4ee818 in TChDirDialog::setUpDialog() ~/source/tvision/tchdrdlg.cpp:158
    #2 0x4ebc8d in TChDirDialog::TChDirDialog(unsigned short, unsigned short) ~/source/tvision/tchdrdlg.cpp:69
    #3 0x41d261 in TVDemo::changeDir() ~/examples/tvdemo/tvdemo2.cpp:225
    #4 0x41a47b in TVDemo::handleEvent(TEvent&) ~/examples/tvdemo/tvdemo2.cpp:89
    #5 0x555179 in TGroup::execute() ~/source/tvision/tgroup.cpp:181
    #6 0x5f010e in TProgram::run() ~/source/tvision/tprogram.cpp:312
    #7 0x4119df in main ~/examples/tvdemo/tvdemo1.cpp:62
    #8 0x7f2289c2a1ef in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #9 0x7f2289c2a2b8 in __libc_start_main_impl ../csu/libc-start.c:360
    #10 0x408094 in _start ../sysdeps/x86_64/start.S:115

0x507000000a74 is located 0 bytes after 68-byte region [0x507000000a30,0x507000000a74)
allocated by thread T0 here:
    #0 0x7f228acfc338 in operator new[](unsigned long) (/lib64/libasan.so.8+0xfc338) (BuildId: 183f53e480d86b3f4e532dc084c6a3c77ec09062)
    #1 0x57262d in TInputLine::TInputLine(TRect const&, unsigned int, TValidator*, unsigned short) ~/source/tvision/tinputli.cpp:84
    #2 0x4e9619 in TChDirDialog::TChDirDialog(unsigned short, unsigned short) ~/source/tvision/tchdrdlg.cpp:50
    #3 0x41d261 in TVDemo::changeDir() ~/examples/tvdemo/tvdemo2.cpp:225
    #4 0x41a47b in TVDemo::handleEvent(TEvent&) ~/examples/tvdemo/tvdemo2.cpp:89
    #5 0x555179 in TGroup::execute() ~/source/tvision/tgroup.cpp:181
    #6 0x5f010e in TProgram::run() ~/source/tvision/tprogram.cpp:312
    #7 0x4119df in main ~/examples/tvdemo/tvdemo1.cpp:62
    #8 0x7f2289c2a1ef in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

            trimEndSeparator( curDir );
**          strcpy( dirInput->data, curDir );
            dirInput->drawView();

After cherry-picking d0d645f onto d1fa783, ASAN still flags the very same line.

[magiblot]

Hi everyone!

First of all, @MookThompson, excuse me for not having replied in a very long time. Thanks for sharing your solution. In my opinion, TChDirDialog implements an outdated concept that no modern user expects to find in (barely) any application these days. That's why I didn't bother integrating Unix paths into it. I still think it's not worth spending much effort into it.

However, I agree that there is no point in displaying drive letters. I fixed this in 545bc7d. Switching from backslashes to forward slashes requires making deeper changes, so I am skipping that for now. Compatibility with Windows paths is tied to the public specification of the Turbo Vision-provided path manipulation functions (fexpand, getCurDir...). Since it is highly discouraged to use these functions anyway (e.g. they rely on buffers limited to MAXPATH characters), I prefer to at least preserve their expected behavior. For example, getCurDir is expected to return a path including a drive letter, so the changes in your solution could break code elsewhere.

Secondly, @jengelh, thanks for the report although the crash you described was not related to the changes in this pull request. Nevertheless, I attempted to fix it in e5b8d3f, although I'm afraid that TChDirDialog (and all the code related to file paths in general) is still vulnerable to memory safety issues due to the prevalence of unsafe string manipulation operations.

Cheers.

[MookThompson]

Hi Magiblot, Thanks for looking at it. I don't blame you for kicking this particular can down the road - it definitely seems like one of those threads you start pulling on that will cause lots of stuff to unravel 🙂. I agree that using TChDirDialog to change the working directory of a process is exceptionally rare nowadays, but I was trying to use (misuse?) it as the basis for a 'Choose directory' dialog, which is still a common requirement (SHBrowseForFolder is the equivalent Windows Common Control). I've ended up hacking together my own dialog that does this specific job in a cross-platform way (along with a File Open dialog that's more like the modern Windows one), so it's not an issue for me. Cheers, Mark

…

________________________________ From: magiblot ***@***.***> Sent: 22 May 2024 00:36 To: magiblot/tvision ***@***.***> Cc: MookThompson ***@***.***>; Mention ***@***.***> Subject: Re: [magiblot/tvision] Makes TChDirDialog on Linux handle native Linux paths (and fix crash) (PR #141) Hi everyone! First of all, @MookThompson<https://github.com/MookThompson>, excuse me for not having replied in a very long time. Thanks for sharing your solution. In my opinion, TChDirDialog implements an outdated concept that no modern user expects to find in (barely) any application these days. That's why I didn't bother integrating Unix paths into it. I still think it's not worth spending much effort into it. However, I agree that there is no point in displaying drive letters. I fixed this in 545bc7d<545bc7d>. Switching from backslashes to forward slashes requires making deeper changes, so I am skipping that for now. Compatibility with Windows paths is tied to the public specification of the Turbo Vision-provided path manipulation functions (fexpand, getCurDir...). Since it is highly discouraged to use these functions anyway (e.g. they rely on buffers limited to MAXPATH characters), I prefer to at least preserve their expected behavior. For example, getCurDir is expected to return a path including a drive letter, so the changes in your solution could break code elsewhere. Secondly, @jengelh<https://github.com/jengelh>, thanks for the report although the crash you described was not related to the changes in this pull request. Nevertheless, I attempted to fix it in e5b8d3f<e5b8d3f>, although I'm afraid that TChDirDialog (and all the code related to file paths in general) is still vulnerable to memory safety issues due to the prevalence of unsafe string manipulation operations. Cheers. — Reply to this email directly, view it on GitHub<#141 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AN6NQ5XPUK26YIHO342O5TTZDPK6ZAVCNFSM6AAAAABCUQIIZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRTGU4TKMZRGQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>