wxIsAbsolutePath function is affected by a potential bug

wxtrac commented 5 years ago

Issue migrated from trac ticket # 18214

component: base | priority: normal | keywords: simple

2018-09-06 08:30:33: parisrossano (Rossano Paris) created the issue

I suggest to introduce a modification for the function called: bool wxIsAbsolutePath (const wxString& filename)

-*File reference ... \src\common\filefn.cpp Line number 329**

Without the suggested modification, using such function passing a file name with length less than 2 chars, it results in an application crash

-*current code**

// MSDOS like
if (filename[0] ## wxT('\\') || (wxIsalpha(filename[0]) && filename[1] wxT(':')))

-*suggested one adding (filename.Length() > 1)**

// MSDOS like
if (filename[0] ## wxT('\\') || ((filename.Length() > 1) && wxIsalpha(filename[0]) && filename[1] wxT(':')))

wxtrac commented 5 years ago

2018-09-06 13:09:44: @vadz changed status from new to confirmed

2018-09-06 13:09:44: @vadz changed type from optimization to defect

2018-09-06 13:09:44: @vadz commented

Yes, this needs to be fixed, but I think the best way to fix it is to just replace this function implementation with wxFileName(filename).IsAbsolute() call.

And for now I'd just recommend using wxFileName::IsAbsolute() instead of this legacy function in your code.

wxtrac commented 5 years ago

2018-09-06 18:47:11: parisrossano (Rossano Paris) commented

Replying to [comment:1 vadz]:

Yes, this needs to be fixed, but I think the best way to fix it is to just replace this function implementation with wxFileName(filename).IsAbsolute() call.

And for now I'd just recommend using wxFileName::IsAbsolute() instead of this legacy function in your code.

Thank you !

wxtrac commented 5 years ago

2018-09-18 19:03:27: neis (Stefan Neis) commented

Replying to [comment:1 vadz]:

Yes, this needs to be fixed, but I think the best way to fix it is to just replace this function implementation with wxFileName(filename).IsAbsolute() call.

Unfortunately, the two function have different ideas of what "absolute path" actually means, AFAICS. I.e. wxIsAbsolutePath("\\something"), wxIsAbsolutePath("/something") and wxIsAbsolutePath("d:something") all seem to return true, while wxFileName(...).IsAbsolute seems to return false for all of these. I feel some sympathy for wanting to treat "\\something" as absolute (and in a scenario with just one drive, you might argue that the error with respect to "d:something" isn't relevant), so I'm not sure how well changing semantics of one of those functions is going to be received.

wxtrac commented 5 years ago

2018-09-18 19:14:37: @vadz commented

None of \\something, /something or d:something are absolute paths under MSW by any definition I know, so I think we can justify changing wxIsAbsolutePath() behaviour perfectly well by just saying that we fixed a bug in it.

wxtrac commented 5 years ago

2018-09-18 21:06:03: @PBfordev commented

Replying to [comment:4 vadz]:

None of \\something, /something or d:something are absolute paths under MSW by any definition I know,

Sorry, I am probably missing something but ::IsPathRelative() seems to disagree with the above.

Running this on Windows 10 v1803

#include <wx/wx.h>
#include <shlwapi.h>

void TestPath(const wxString& path)
{       
    wxLogMessage("Path \'%s\' is %s.", path, ::PathIsRelative(path.fn_str()) ? "relative" : "absolute");
}

class MyApp : public wxApp
{
public: 
    bool OnInit()
    {
        new wxLogWindow(NULL, "Test path");

        TestPath("\\Something");
        TestPath("/Something");
        TestPath("C:Something");

        return true;
    }
}; wxIMPLEMENT_APP(MyApp);

produced 20:56:18: Path '\Something' is absolute. 20:56:18: Path '/Something' is relative. 20:56:18: Path 'C:Something' is absolute.

TBH, at least cases 1 and 3 make sense to me, taking the CWD into account.

wxtrac commented 5 years ago

2018-09-18 22:11:04: @vadz commented

I didn't know about ::IsPathRelative() but, to be fair, if wxIsAbsolutePath() can be buggy, why can't it too?

"Taking CWD into account" makes any path absolute, doesn't it? I agree that Windows/DOS paths are confusing (and even Raymond Chen can't explain them 100% clearly), but IMO there is no doubt that neither \something nor c:something can possibly be absolute as they depend on working drive or current working directory of the drive "c:", which makes them relative almost by definition...

wxtrac commented 5 years ago

2018-09-18 22:58:38: @PBfordev commented

Sorry, I mistyped the function name, I meant Windows Shell PathIsRelative, see the test code. And I even wrote it before rediscovering Raymond's article who for some reason has it wrong as well?!

Regarding its possible bugginess: I suppose it works as documented on the MSDN, I guess even the definition of "relative" is relative. :P

I just wanted to address that the statement

None of \\something, /something or d:something are absolute paths under MSW by any definition I know,

may not match the official documentation regarding MSW paths, for purpose of passing the path to Windows file functions.

BTW, I find wxTrac toggling the ticket Cc with every post a person makes somewhat annoying, looks more like a bug than a feature to me.

wxtrac commented 5 years ago

2018-09-18 23:37:10: @vadz commented

If you want to see the full treatment of Windows file paths, I don't think you can find anything better than this article. Beware, it will take time to read it and I'm not sure if it changes anything in practice. But I remember finding it quite informative.

Anyhow, the real question is why would a typical application want to use wxIsAbsolutePath() or wxFileName::IsAbsolute() in the first place. I'm not sure I actually see any rationale for using these functions at all...

BTW, I find wxTrac toggling the ticket Cc with every post a person makes somewhat annoying, looks more like a bug than a feature to me.

I had no idea it did this (it doesn't for me...). I'm afraid I don't know what, if anything, can be done about it, sorry.

wxtrac commented 5 years ago

2018-09-19 07:31:34: @PBfordev commented

All I wanted to say is point out the Windows "canon".

I am simple so I have always used simplified "a relative path is a path that needs to be prepended with another path to make it CWD-independent" definition.

wxWidgets / wxWidgets