| Message ID | 20210306130744.780447-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 9665fd31e8f5d9738e742fcdf0e76ea9ce76b049 |
| Headers | show |
| Series | [FFmpeg-devel] avformat/http: Fix leak when using deprecated option | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On Sat, 6 Mar 2021, Andreas Rheinhardt wrote: > When the deprecated option "user-agent" was set to something different > than its default value, said option would always precede and overwrite > the ordinary user_agent option (regardless of whether it was explicitly > set) which leads to a leak of the user_agent option (which has a default > value, so the leak happens always). > Fix this by setting the same destination for both options; the last > option applied wins then. I think you should simply remove the old option. AFAIK public options are not considered part of API/ABI, therefore after the deprecation period of at least 2 years they can be removed anytime. I am not sure why a deprecation guard was added for this removal. Regards, Marton > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/http.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > diff --git a/libavformat/http.c b/libavformat/http.c > index c5df1def62..d44bc64f7b 100644 > --- a/libavformat/http.c > +++ b/libavformat/http.c > @@ -78,9 +78,6 @@ typedef struct HTTPContext { > char *http_version; > char *user_agent; > char *referer; > -#if FF_API_HTTP_USER_AGENT > - char *user_agent_deprecated; > -#endif > char *content_type; > /* Set if the server correctly handles Connection: close and will close > * the connection after feeding us the content. */ > @@ -145,7 +142,7 @@ static const AVOption options[] = { > { "user_agent", "override User-Agent header", OFFSET(user_agent), AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D }, > { "referer", "override referer header", OFFSET(referer), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D }, > #if FF_API_HTTP_USER_AGENT > - { "user-agent", "use the \"user_agent\" option instead", OFFSET(user_agent_deprecated), AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D|AV_OPT_FLAG_DEPRECATED }, > + { "user-agent", "use the \"user_agent\" option instead", OFFSET(user_agent), AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D|AV_OPT_FLAG_DEPRECATED }, > #endif > { "multiple_requests", "use persistent connections", OFFSET(multiple_requests), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, D | E }, > { "post_data", "set custom HTTP post data", OFFSET(post_data), AV_OPT_TYPE_BINARY, .flags = D | E }, > @@ -1321,12 +1318,6 @@ static int http_connect(URLContext *h, const char *path, const char *local_path, > } > } > > -#if FF_API_HTTP_USER_AGENT > - if (strcmp(s->user_agent_deprecated, DEFAULT_USER_AGENT)) { > - s->user_agent = av_strdup(s->user_agent_deprecated); > - } > -#endif > - > av_bprintf(&request, "%s ", method); > bprint_escaped_path(&request, path); > av_bprintf(&request, " HTTP/1.1\r\n"); > -- > 2.27.0 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 3/6/2021 1:57 PM, Marton Balint wrote: > > > On Sat, 6 Mar 2021, Andreas Rheinhardt wrote: > >> When the deprecated option "user-agent" was set to something different >> than its default value, said option would always precede and overwrite >> the ordinary user_agent option (regardless of whether it was explicitly >> set) which leads to a leak of the user_agent option (which has a default >> value, so the leak happens always). >> Fix this by setting the same destination for both options; the last >> option applied wins then. > > I think you should simply remove the old option. AFAIK public options > are not considered part of API/ABI, therefore after the deprecation > period of at least 2 years they can be removed anytime. I am not sure > why a deprecation guard was added for this removal. They have historically been removed after a deprecation period. There is a AV_OPT_FLAG_DEPRECATED flag for that purpose after all. Also, new options tend to come with a micro version bump. > > Regards, > Marton > >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/http.c | 11 +---------- >> 1 file changed, 1 insertion(+), 10 deletions(-) >> >> diff --git a/libavformat/http.c b/libavformat/http.c >> index c5df1def62..d44bc64f7b 100644 >> --- a/libavformat/http.c >> +++ b/libavformat/http.c >> @@ -78,9 +78,6 @@ typedef struct HTTPContext { >> char *http_version; >> char *user_agent; >> char *referer; >> -#if FF_API_HTTP_USER_AGENT >> - char *user_agent_deprecated; >> -#endif >> char *content_type; >> /* Set if the server correctly handles Connection: close and will >> close >> * the connection after feeding us the content. */ >> @@ -145,7 +142,7 @@ static const AVOption options[] = { >> { "user_agent", "override User-Agent header", OFFSET(user_agent), >> AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D }, >> { "referer", "override referer header", OFFSET(referer), >> AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D }, >> #if FF_API_HTTP_USER_AGENT >> - { "user-agent", "use the \"user_agent\" option instead", >> OFFSET(user_agent_deprecated), AV_OPT_TYPE_STRING, { .str = >> DEFAULT_USER_AGENT }, 0, 0, D|AV_OPT_FLAG_DEPRECATED }, >> + { "user-agent", "use the \"user_agent\" option instead", >> OFFSET(user_agent), AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, >> 0, 0, D|AV_OPT_FLAG_DEPRECATED }, >> #endif >> { "multiple_requests", "use persistent connections", >> OFFSET(multiple_requests), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, D | E }, >> { "post_data", "set custom HTTP post data", OFFSET(post_data), >> AV_OPT_TYPE_BINARY, .flags = D | E }, >> @@ -1321,12 +1318,6 @@ static int http_connect(URLContext *h, const >> char *path, const char *local_path, >> } >> } >> >> -#if FF_API_HTTP_USER_AGENT >> - if (strcmp(s->user_agent_deprecated, DEFAULT_USER_AGENT)) { >> - s->user_agent = av_strdup(s->user_agent_deprecated); >> - } >> -#endif >> - >> av_bprintf(&request, "%s ", method); >> bprint_escaped_path(&request, path); >> av_bprintf(&request, " HTTP/1.1\r\n"); >> -- >> 2.27.0 >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
James Almer: > On 3/6/2021 1:57 PM, Marton Balint wrote: >> >> >> On Sat, 6 Mar 2021, Andreas Rheinhardt wrote: >> >>> When the deprecated option "user-agent" was set to something different >>> than its default value, said option would always precede and overwrite >>> the ordinary user_agent option (regardless of whether it was explicitly >>> set) which leads to a leak of the user_agent option (which has a default >>> value, so the leak happens always). >>> Fix this by setting the same destination for both options; the last >>> option applied wins then. >> >> I think you should simply remove the old option. AFAIK public options >> are not considered part of API/ABI, therefore after the deprecation >> period of at least 2 years they can be removed anytime. I am not sure >> why a deprecation guard was added for this removal. > > They have historically been removed after a deprecation period. There is > a AV_OPT_FLAG_DEPRECATED flag for that purpose after all. > If that's the case, I can prepone the removal of a lot of options. Will immediately prepare patches. > Also, new options tend to come with a micro version bump. > >> >> Regards, >> Marton >> >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> libavformat/http.c | 11 +---------- >>> 1 file changed, 1 insertion(+), 10 deletions(-) >>> >>> diff --git a/libavformat/http.c b/libavformat/http.c >>> index c5df1def62..d44bc64f7b 100644 >>> --- a/libavformat/http.c >>> +++ b/libavformat/http.c >>> @@ -78,9 +78,6 @@ typedef struct HTTPContext { >>> char *http_version; >>> char *user_agent; >>> char *referer; >>> -#if FF_API_HTTP_USER_AGENT >>> - char *user_agent_deprecated; >>> -#endif >>> char *content_type; >>> /* Set if the server correctly handles Connection: close and will >>> close >>> * the connection after feeding us the content. */ >>> @@ -145,7 +142,7 @@ static const AVOption options[] = { >>> { "user_agent", "override User-Agent header", OFFSET(user_agent), >>> AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D }, >>> { "referer", "override referer header", OFFSET(referer), >>> AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D }, >>> #if FF_API_HTTP_USER_AGENT >>> - { "user-agent", "use the \"user_agent\" option instead", >>> OFFSET(user_agent_deprecated), AV_OPT_TYPE_STRING, { .str = >>> DEFAULT_USER_AGENT }, 0, 0, D|AV_OPT_FLAG_DEPRECATED }, >>> + { "user-agent", "use the \"user_agent\" option instead", >>> OFFSET(user_agent), AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT >>> }, 0, 0, D|AV_OPT_FLAG_DEPRECATED }, >>> #endif >>> { "multiple_requests", "use persistent connections", >>> OFFSET(multiple_requests), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, D | >>> E }, >>> { "post_data", "set custom HTTP post data", OFFSET(post_data), >>> AV_OPT_TYPE_BINARY, .flags = D | E }, >>> @@ -1321,12 +1318,6 @@ static int http_connect(URLContext *h, const >>> char *path, const char *local_path, >>> } >>> } >>> >>> -#if FF_API_HTTP_USER_AGENT >>> - if (strcmp(s->user_agent_deprecated, DEFAULT_USER_AGENT)) { >>> - s->user_agent = av_strdup(s->user_agent_deprecated); >>> - } >>> -#endif >>> - >>> av_bprintf(&request, "%s ", method); >>> bprint_escaped_path(&request, path); >>> av_bprintf(&request, " HTTP/1.1\r\n"); >>> -- >>> 2.27.0 >>>
Marton Balint: > > > On Sat, 6 Mar 2021, Andreas Rheinhardt wrote: > >> When the deprecated option "user-agent" was set to something different >> than its default value, said option would always precede and overwrite >> the ordinary user_agent option (regardless of whether it was explicitly >> set) which leads to a leak of the user_agent option (which has a default >> value, so the leak happens always). >> Fix this by setting the same destination for both options; the last >> option applied wins then. > > I think you should simply remove the old option. AFAIK public options > are not considered part of API/ABI, therefore after the deprecation > period of at least 2 years they can be removed anytime. I am not sure > why a deprecation guard was added for this removal. > But I think I need to apply this anyway in order to have something to backport (I don't think we remove options from old releases). > Regards, > Marton > >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/http.c | 11 +---------- >> 1 file changed, 1 insertion(+), 10 deletions(-) >> >> diff --git a/libavformat/http.c b/libavformat/http.c >> index c5df1def62..d44bc64f7b 100644 >> --- a/libavformat/http.c >> +++ b/libavformat/http.c >> @@ -78,9 +78,6 @@ typedef struct HTTPContext { >> char *http_version; >> char *user_agent; >> char *referer; >> -#if FF_API_HTTP_USER_AGENT >> - char *user_agent_deprecated; >> -#endif >> char *content_type; >> /* Set if the server correctly handles Connection: close and will >> close >> * the connection after feeding us the content. */ >> @@ -145,7 +142,7 @@ static const AVOption options[] = { >> { "user_agent", "override User-Agent header", OFFSET(user_agent), >> AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D }, >> { "referer", "override referer header", OFFSET(referer), >> AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D }, >> #if FF_API_HTTP_USER_AGENT >> - { "user-agent", "use the \"user_agent\" option instead", >> OFFSET(user_agent_deprecated), AV_OPT_TYPE_STRING, { .str = >> DEFAULT_USER_AGENT }, 0, 0, D|AV_OPT_FLAG_DEPRECATED }, >> + { "user-agent", "use the \"user_agent\" option instead", >> OFFSET(user_agent), AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, >> 0, 0, D|AV_OPT_FLAG_DEPRECATED }, >> #endif >> { "multiple_requests", "use persistent connections", >> OFFSET(multiple_requests), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, D | E }, >> { "post_data", "set custom HTTP post data", OFFSET(post_data), >> AV_OPT_TYPE_BINARY, .flags = D | E }, >> @@ -1321,12 +1318,6 @@ static int http_connect(URLContext *h, const >> char *path, const char *local_path, >> } >> } >> >> -#if FF_API_HTTP_USER_AGENT >> - if (strcmp(s->user_agent_deprecated, DEFAULT_USER_AGENT)) { >> - s->user_agent = av_strdup(s->user_agent_deprecated); >> - } >> -#endif >> - >> av_bprintf(&request, "%s ", method); >> bprint_escaped_path(&request, path); >> av_bprintf(&request, " HTTP/1.1\r\n"); >> -- >> 2.27.0 >> >> _______________________________________________ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Sat, 6 Mar 2021, Andreas Rheinhardt wrote: > Marton Balint: >> >> >> On Sat, 6 Mar 2021, Andreas Rheinhardt wrote: >> >>> When the deprecated option "user-agent" was set to something different >>> than its default value, said option would always precede and overwrite >>> the ordinary user_agent option (regardless of whether it was explicitly >>> set) which leads to a leak of the user_agent option (which has a default >>> value, so the leak happens always). >>> Fix this by setting the same destination for both options; the last >>> option applied wins then. >> >> I think you should simply remove the old option. AFAIK public options >> are not considered part of API/ABI, therefore after the deprecation >> period of at least 2 years they can be removed anytime. I am not sure >> why a deprecation guard was added for this removal. >> > But I think I need to apply this anyway in order to have something to > backport (I don't think we remove options from old releases). Yeah, that is true. Thanks, Marton
diff --git a/libavformat/http.c b/libavformat/http.c index c5df1def62..d44bc64f7b 100644 --- a/libavformat/http.c +++ b/libavformat/http.c @@ -78,9 +78,6 @@ typedef struct HTTPContext { char *http_version; char *user_agent; char *referer; -#if FF_API_HTTP_USER_AGENT - char *user_agent_deprecated; -#endif char *content_type; /* Set if the server correctly handles Connection: close and will close * the connection after feeding us the content. */ @@ -145,7 +142,7 @@ static const AVOption options[] = { { "user_agent", "override User-Agent header", OFFSET(user_agent), AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D }, { "referer", "override referer header", OFFSET(referer), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, D }, #if FF_API_HTTP_USER_AGENT - { "user-agent", "use the \"user_agent\" option instead", OFFSET(user_agent_deprecated), AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D|AV_OPT_FLAG_DEPRECATED }, + { "user-agent", "use the \"user_agent\" option instead", OFFSET(user_agent), AV_OPT_TYPE_STRING, { .str = DEFAULT_USER_AGENT }, 0, 0, D|AV_OPT_FLAG_DEPRECATED }, #endif { "multiple_requests", "use persistent connections", OFFSET(multiple_requests), AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, D | E }, { "post_data", "set custom HTTP post data", OFFSET(post_data), AV_OPT_TYPE_BINARY, .flags = D | E }, @@ -1321,12 +1318,6 @@ static int http_connect(URLContext *h, const char *path, const char *local_path, } } -#if FF_API_HTTP_USER_AGENT - if (strcmp(s->user_agent_deprecated, DEFAULT_USER_AGENT)) { - s->user_agent = av_strdup(s->user_agent_deprecated); - } -#endif - av_bprintf(&request, "%s ", method); bprint_escaped_path(&request, path); av_bprintf(&request, " HTTP/1.1\r\n");
When the deprecated option "user-agent" was set to something different than its default value, said option would always precede and overwrite the ordinary user_agent option (regardless of whether it was explicitly set) which leads to a leak of the user_agent option (which has a default value, so the leak happens always). Fix this by setting the same destination for both options; the last option applied wins then. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/http.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)