Skip to content
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.

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

CUtil::ValidatePath fix double (back)slash check #15499

Merged
merged 2 commits into from Feb 21, 2019

Conversation

Projects
None yet
2 participants
@jvandenbroek
Copy link
Contributor

commented Feb 12, 2019

Very simple patch for correct double (back)slash path validation. This is only used by the function called from Python (xbmc.validatePath), all others in code assume false and skip this section. That is probably why this has been unnoticed for such a long time, but an user of my add-on tried to validate a path like this against it: nfs://1.2.3.4//video/Test/test.avi. The current situation returns 'nfs://1.2.3.4' instead of the full path with only the doubles removed. Of course it seems unnecessary to use the function at all, but it's nice to have it fixed anyway (or it should be removed if obsolete).

When we're looking at the previous commit, already 5 years ago, it seems clear that .Delete(x) has been replaced by .erase(x). However that Delete function used to call erase(x, length) with a default value of 1 for length. So that's when this bug got introduced. Simply adding a length of 1 to the erase() function will fix this.

This is my first PR on this repo, so hopefully I've done nothing wrong ;-) Otherwise, please forgive me.

@pkerling

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Welcome to the repo! :-)

This is my first PR on this repo, so hopefully I've done nothing wrong ;-) Otherwise, please forgive me.

Please do not delete the PR template that we provide, but fill it out instead.

Code looks fine (I didn't investigate the logic yet) - two things that I would like to see:

  • Explain the change in the commit message (similarly to the PR description)
  • Add a test case for this change, it should be pretty simple here since it is a self-contained utility function
CUtil::ValidatePath fix stripping double (back)slash chars
Only used by Python call xbmc.validatePath(). When double slash is found in supplied path, the remaining of the path is stripped of instead of just the double slash. This used to work as intended before the previous change, until Delete(x) got replaced by erase(x). Delete(x) was a function which called erase(x, 1).

@jvandenbroek jvandenbroek force-pushed the jvandenbroek:fix_double_slash_validatePath branch from e2da5aa to ae906da Feb 12, 2019

@jvandenbroek

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

@pkerling Thanks for your instructions. Sorry for deleting the template, I just looked at some other PR and didn't see it, so I though for such a simple patch it wasn't needed. Do you want me to provide it anyway, or is it fine to left it out for this one?

I've added some more detail to the commit message, so it's more clear what it does.

Test case:

path = "nfs://1.2.3.4//Test/test.avi"
path = xbmc.validatePath(path)

# Current result:
path == "nfs://1.2.3.4"

# Patched result:
path == "nfs://1.2.3.4/Test/test.avi" 
@pkerling

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

I just looked at some other PR and didn't see it

The PR template is mostly for new contributors (such as yourself) to make sure you know about our submission process etc. In fact, you would have seen the "I have added tests to cover my change" point in the checklist if you had used it.

such a simple patch

Even a one-line change in an innocent-looking utility function can kill the whole application :-)

Do you want me to provide it anyway, or is it fine to left it out for this one?

Just make sure to read it through next time.

I've added some more detail to the commit message, so it's more clear what it does.

Thanks!

Test case:

What I meant was that you should add a unit test to our test suite so that we never regress on the correct behavior by accident. You can add it to this file: https://github.com/xbmc/xbmc/blob/master/xbmc/test/TestUtil.cpp - it should be mostly self-explanatory from reading the code that is there but do ask if you need any help.

@jvandenbroek

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

Thanks, I've added the test. Not sure if all is covered and how to test them myself, so hopefully I didn't make any mistake 😅

@pkerling

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

On Linux or Mac OS, try running make check :-) On Windows there should be a button in the IDE somewhere I guess...

@jvandenbroek

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

On Linux test seems to pass:

        Start 517: TestUtil.ValidatePath
517/522 Test #517: TestUtil.ValidatePath ....................................................   Passed    0.11 sec
@pkerling

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

lgtm, thanks for the effort!

@pkerling pkerling added this to the Leia 18.2-rc1 milestone Feb 21, 2019

@pkerling pkerling merged commit d01232e into xbmc:master Feb 21, 2019

1 check passed

default You're awesome. Have a cookie
Details

@jvandenbroek jvandenbroek deleted the jvandenbroek:fix_double_slash_validatePath branch Mar 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.