Opened 4 years ago

Closed 4 years ago

#8814 closed defect (fixed)

Can't open HTTP URL whose file name contains ".."

Reported by: Josef Zlomek Owned by: Steven Liu
Priority: normal Component: avformat
Version: git-master Keywords: http
Cc: josef@pex.com, liuqi05@kuaishou.com, Marton Balint Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

Summary of the bug:

FFmpeg can't open HTTP URL whose file name contains ".." as in my example below.
The problem is that the function trim_double_dot_url does not handle ".." inside a name correctly. It should eliminate just directory "..".

How to reproduce:

% ffplay  'https://traffic.libsyn.com/secure/radicalpersonalfinance/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3?dest-id=152630'

ffmpeg version master (4.3.1)
built on Linux

Patches should be submitted to the ffmpeg-devel mailing list and not this bug tracker.

Backtrace:
#0 trim_double_dot_url (buf=0x7fffe94fb070 "", rel=0x7fffe94fc12a "https://hwcdn.libsyn.com/p/3/a/f/3afabfbd7783eb57/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3?c_id=7"..., size=4096) at libavformat/url.c:83
#1 0x00007ffff73bbae7 in ff_make_absolute_url (buf=0x7fffe94fb070 "", size=4096, base=0x7fffd0004040 "https://traffic.libsyn.com/secure/radicalpersonalfinance/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3"...,

rel=0x7fffe94fc12a "https://hwcdn.libsyn.com/p/3/a/f/3afabfbd7783eb57/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3?c_id=7"...) at libavformat/url.c:156

#2 0x00007ffff7281ca6 in parse_location (s=0x7fffd00016b0, p=0x7fffe94fc12a "https://hwcdn.libsyn.com/p/3/a/f/3afabfbd7783eb57/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3?c_id=7"...) at libavformat/http.c:679
#3 0x00007ffff7282cf1 in process_line (h=0x7fffd0001570, line=0x7fffe94fc120 "location", line_count=3, new_location=0x7fffe94fd5f8) at libavformat/http.c:1004
#4 0x00007ffff7283672 in http_read_header (h=0x7fffd0001570, new_location=0x7fffe94fd5f8) at libavformat/http.c:1168
#5 0x00007ffff7284287 in http_connect (h=0x7fffd0001570, path=0x7fffe94ffa00 "/secure/radicalpersonalfinance/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3?dest-id=152630",

local_path=0x7fffe94ffa00 "/secure/radicalpersonalfinance/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3?dest-id=152630", hoststr=0x7fffe9501210 "traffic.libsyn.com", auth=0x7fffe9500e00 "", proxyauth=0x7fffe9500a00 "", new_location=0x7fffe94fd5f8)
at libavformat/http.c:1369

#6 0x00007ffff7280a27 in http_open_cnx_internal (h=0x7fffd0001570, options=0x7fffe9501ce0) at libavformat/http.c:253
#7 0x00007ffff7280ab7 in http_open_cnx (h=0x7fffd0001570, options=0x7fffe9501ce0) at libavformat/http.c:273
#8 0x00007ffff728193d in http_open (h=0x7fffd0001570, uri=0x7fffd00015d0 "https://traffic.libsyn.com/secure/radicalpersonalfinance/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3"..., flags=1, options=0x7fffe9501ce0) at libavformat/http.c:590
#9 0x00007ffff7221005 in ffurl_connect (uc=0x7fffd0001570, options=0x7fffe9501ce0) at libavformat/avio.c:210
#10 0x00007ffff72216f4 in ffurl_open_whitelist (puc=0x7fffe9501bc0, filename=0x7fdb40 "https://traffic.libsyn.com/secure/radicalpersonalfinance/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3"..., flags=1, int_cb=0x7fffd0001008, options=0x7fffe9501ce0,

whitelist=0x0, blacklist=0x0, parent=0x0) at libavformat/avio.c:348

#11 0x00007ffff722545b in ffio_open_whitelist (s=0x7fffd0000b60, filename=0x7fdb40 "https://traffic.libsyn.com/secure/radicalpersonalfinance/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3"..., flags=1, int_cb=0x7fffd0001008, options=0x7fffe9501ce0, whitelist=0x0,

blacklist=0x0) at libavformat/aviobuf.c:1142

#12 0x00007ffff7345e5e in io_open_default (s=0x7fffd0000b40, pb=0x7fffd0000b60, url=0x7fdb40 "https://traffic.libsyn.com/secure/radicalpersonalfinance/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3"..., flags=1, options=0x7fffe9501ce0) at libavformat/options.c:188
#13 0x00007ffff73bd22d in init_input (s=0x7fffd0000b40, filename=0x7fdb40 "https://traffic.libsyn.com/secure/radicalpersonalfinance/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3"..., options=0x7fffe9501ce0) at libavformat/utils.c:432
#14 0x00007ffff73bd740 in avformat_open_input (ps=0x7fffe9501e20, filename=0x7fdb40 "https://traffic.libsyn.com/secure/radicalpersonalfinance/2020-07-20_Friday_QA-Equity_Stripping_a_House_in_Divorce_Buy_Whole_Life_Insurance_in_a_Sabbatical_Affirmations_in_Personal_Development_Etc..mp3"..., fmt=0x0, options=0x620bc8 <format_opts>) at libavformat/utils.c:569
#15 0x000000000041615f in read_thread (arg=0x1e0c980) at fftools/ffplay.c:2792
#16 0x00007ffff457e8dc in ?? () from /usr/lib64/libSDL2-2.0.so.0
#17 0x00007ffff45f31d9 in ?? () from /usr/lib64/libSDL2-2.0.so.0
#18 0x00007ffff42ff4f9 in start_thread () from /lib64/libpthread.so.0
#19 0x00007ffff4037f2f in clone () from /lib64/libc.so.6

Change History (11)

comment:1 by Steven Liu, 4 years ago

Cc: liuqi05@kuaishou.com added
Owner: set to Steven Liu
Status: newopen

comment:2 by Josef Zlomek, 4 years ago

I think this would not work if the file name ends with "..", for example "xyz.."
I guess we need to test if ".." is at the beginning of the string or after "/",
and ".." is at the end of the string or before "/".

in reply to:  2 comment:3 by Steven Liu, 4 years ago

Replying to jzlomek:

I think this would not work if the file name ends with "..", for example "xyz.."
I guess we need to test if ".." is at the beginning of the string or after "/",
and ".." is at the end of the string or before "/".

Just try it please.
i think it should work, because just check the .. is directory or file.
but i think it maybe have safety problem, i will check there have next byte by strlen(node).

comment:4 by Josef Zlomek, 4 years ago

The patchset does not wrok well, as I suggested before.
Testcase:

test("http://server/foo/bar", "aa/bb/cc/dd/ee../other/url/a.mp3");

gives:

http://server/foo/bar aa/bb/cc/dd/ee../other/url/a.mp3 => http://server/foo/aa/bb/cc/dd/other/url/a.mp3

but should be

http://server/foo/bar aa/bb/cc/dd/ee../other/url/a.mp3 => http://server/foo/aa/bb/cc/dd/ee../other/url/a.mp3

in reply to:  4 comment:5 by Steven Liu, 4 years ago

Replying to jzlomek:

The patchset does not wrok well, as I suggested before.
Testcase:

test("http://server/foo/bar", "aa/bb/cc/dd/ee../other/url/a.mp3");

gives:

http://server/foo/bar aa/bb/cc/dd/ee../other/url/a.mp3 => http://server/foo/aa/bb/cc/dd/other/url/a.mp3

but should be

http://server/foo/bar aa/bb/cc/dd/ee../other/url/a.mp3 => http://server/foo/aa/bb/cc/dd/ee../other/url/a.mp3

try this patch please:
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1853

comment:6 by Josef Zlomek, 4 years ago

It is better but still not correct. Consider this test:

test("http://server/foo/bar", "a/b/../c/d/../e../.../..f/g../h../other/url/a.mp3/...");
It should give "http://server/foo/bar/a/c/e../.../..f/g../h../other/url/a.mp3/...".

I think the best would be to use strtok(p, "/") to split the path into the components and for each ".." component remove the previous one (if there are some still).

in reply to:  6 comment:7 by Steven Liu, 4 years ago

Replying to jzlomek:

It is better but still not correct. Consider this test:

test("http://server/foo/bar", "a/b/../c/d/../e../.../..f/g../h../other/url/a.mp3/...");
It should give "http://server/foo/bar/a/c/e../.../..f/g../h../other/url/a.mp3/...".

I think the best would be to use strtok(p, "/") to split the path into the components and for each ".." component remove the previous one (if there are some still).

What about this one:
https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1871

comment:8 by Josef Zlomek, 4 years ago

PAtchset https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1871
does not work:

  • /foo/bar .. => /
  • /foo/bar/baz .. => /foo/

+ /foo/bar .. => foo/bar/baz
+ /foo/bar/baz .. => /foo
foo/bar/baz

Also this would be nice:

+ http://server/foo/bar a/b/../c/d/../e../..f/.../other/url/.. => http://server/foo/a/c/e../..f/.../other

comment:10 by Carl Eugen Hoyos, 4 years ago

Keywords: http added; URL removed

comment:11 by Marton Balint, 4 years ago

Cc: Marton Balint added
Resolution: fixed
Status: openclosed
Note: See TracTickets for help on using tickets.