| Message ID | 1638365954-29420-1-git-send-email-lance.lmwang@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avformat/rtsp: make use of avio_get_str() to load the sdp | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote: > From: Limin Wang <lance.lmwang@gmail.com> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > --- > libavformat/rtsp.c | 30 ++++++++++++++++++++---------- > 1 file changed, 20 insertions(+), 10 deletions(-) > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c > index e6a4993..ec8be8b 100644 > --- a/libavformat/rtsp.c > +++ b/libavformat/rtsp.c > @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name, > av_strlcatf(buf, size, ",%s", addrs[i]->addr); > } > > +static char *read_sdp_str(AVIOContext *pb, int size) > +{ > + int n; > + char *str; > + > + if (size < 0 || size == INT_MAX) > + return NULL; > + > + str = av_malloc(size + 1); > + if (!str) > + return NULL; > + n = avio_get_str(pb, size, str, size + 1); > + if (n < size) > + avio_skip(pb, size - n); What? Why do you want to skip past data in the input? Size here is just a random static upper limit of how large an SDP file would be. This patch totally lacks a motivation of why you want to do this. // Martin
On Wed, Dec 01, 2021 at 03:55:37PM +0200, Martin Storsjö wrote: > On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote: > > > From: Limin Wang <lance.lmwang@gmail.com> > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > --- > > libavformat/rtsp.c | 30 ++++++++++++++++++++---------- > > 1 file changed, 20 insertions(+), 10 deletions(-) > > > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c > > index e6a4993..ec8be8b 100644 > > --- a/libavformat/rtsp.c > > +++ b/libavformat/rtsp.c > > @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name, > > av_strlcatf(buf, size, ",%s", addrs[i]->addr); > > } > > > > +static char *read_sdp_str(AVIOContext *pb, int size) > > +{ > > + int n; > > + char *str; > > + > > + if (size < 0 || size == INT_MAX) > > + return NULL; > > + > > + str = av_malloc(size + 1); > > + if (!str) > > + return NULL; > > + n = avio_get_str(pb, size, str, size + 1); > > + if (n < size) > > + avio_skip(pb, size - n); > > What? Why do you want to skip past data in the input? Size here is just a > random static upper limit of how large an SDP file would be. yes, my fault, don't need to run avio_skip(). For avio_get_str() will return error only buflen <=0, so str is valid anyway. > > This patch totally lacks a motivation of why you want to do this. considering the SDP is strings in file, so I think it's better to use avio_get_str(). Also, I'm considering to remove limit of SDP_MAX_SIZE after that. > > // Martin >
On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote: > On Wed, Dec 01, 2021 at 03:55:37PM +0200, Martin Storsjö wrote: >> On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote: >> >> > From: Limin Wang <lance.lmwang@gmail.com> >> > >> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> >> > --- >> > libavformat/rtsp.c | 30 ++++++++++++++++++++---------- >> > 1 file changed, 20 insertions(+), 10 deletions(-) >> > >> > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c >> > index e6a4993..ec8be8b 100644 >> > --- a/libavformat/rtsp.c >> > +++ b/libavformat/rtsp.c >> > @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name, >> > av_strlcatf(buf, size, ",%s", addrs[i]->addr); >> > } >> > >> > +static char *read_sdp_str(AVIOContext *pb, int size) >> > +{ >> > + int n; >> > + char *str; >> > + >> > + if (size < 0 || size == INT_MAX) >> > + return NULL; >> > + >> > + str = av_malloc(size + 1); >> > + if (!str) >> > + return NULL; >> > + n = avio_get_str(pb, size, str, size + 1); >> > + if (n < size) >> > + avio_skip(pb, size - n); >> >> What? Why do you want to skip past data in the input? Size here is just a >> random static upper limit of how large an SDP file would be. > yes, my fault, don't need to run avio_skip(). For avio_get_str() will return error only > buflen <=0, so str is valid anyway. > >> >> This patch totally lacks a motivation of why you want to do this. > > considering the SDP is strings in file, so I think it's better to use avio_get_str(). Maybe, but warranting a patch changing things on its own, IMO. > Also, I'm considering to remove limit of SDP_MAX_SIZE after that. If you'd defer this patch to a patchset that does that, it might make more sense. But I don't see much value in applying it on its own. // Martin
On Wed, Dec 01, 2021 at 05:17:08PM +0200, Martin Storsjö wrote: > On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote: > > > On Wed, Dec 01, 2021 at 03:55:37PM +0200, Martin Storsjö wrote: > > > On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote: > > > > > > > From: Limin Wang <lance.lmwang@gmail.com> > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > > > --- > > > > libavformat/rtsp.c | 30 ++++++++++++++++++++---------- > > > > 1 file changed, 20 insertions(+), 10 deletions(-) > > > > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c > > > > index e6a4993..ec8be8b 100644 > > > > --- a/libavformat/rtsp.c > > > > +++ b/libavformat/rtsp.c > > > > @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name, > > > > av_strlcatf(buf, size, ",%s", addrs[i]->addr); > > > > } > > > > > +static char *read_sdp_str(AVIOContext *pb, int size) > > > > +{ > > > > + int n; > > > > + char *str; > > > > + > > > > + if (size < 0 || size == INT_MAX) > > > > + return NULL; > > > > + > > > > + str = av_malloc(size + 1); > > > > + if (!str) > > > > + return NULL; > > > > + n = avio_get_str(pb, size, str, size + 1); > > > > + if (n < size) > > > > + avio_skip(pb, size - n); > > > > > > What? Why do you want to skip past data in the input? Size here is just a > > > random static upper limit of how large an SDP file would be. > > yes, my fault, don't need to run avio_skip(). For avio_get_str() will return error only > > buflen <=0, so str is valid anyway. > > > > > > > > This patch totally lacks a motivation of why you want to do this. > > > > considering the SDP is strings in file, so I think it's better to use avio_get_str(). > > Maybe, but warranting a patch changing things on its own, IMO. > > > Also, I'm considering to remove limit of SDP_MAX_SIZE after that. > > If you'd defer this patch to a patchset that does that, it might make more > sense. But I don't see much value in applying it on its own. Yes, I consider to add new api which is avio_get_str_len() to get the string len from pb, before that, I want to hear no objection to use avio_get_str(). then I'll submit a patchset for the complete change for review. > > // Martin > _______________________________________________ > 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 Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote: > On Wed, Dec 01, 2021 at 05:17:08PM +0200, Martin Storsjö wrote: >> On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote: >> >> > On Wed, Dec 01, 2021 at 03:55:37PM +0200, Martin Storsjö wrote: >> > > On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote: >> > > >> > > > From: Limin Wang <lance.lmwang@gmail.com> >> > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> >> > > > --- >> > > > libavformat/rtsp.c | 30 ++++++++++++++++++++---------- >> > > > 1 file changed, 20 insertions(+), 10 deletions(-) >> > > > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c >> > > > index e6a4993..ec8be8b 100644 >> > > > --- a/libavformat/rtsp.c >> > > > +++ b/libavformat/rtsp.c >> > > > @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name, >> > > > av_strlcatf(buf, size, ",%s", addrs[i]->addr); >> > > > } >> > > > > +static char *read_sdp_str(AVIOContext *pb, int size) >> > > > +{ >> > > > + int n; >> > > > + char *str; >> > > > + >> > > > + if (size < 0 || size == INT_MAX) >> > > > + return NULL; >> > > > + >> > > > + str = av_malloc(size + 1); >> > > > + if (!str) >> > > > + return NULL; >> > > > + n = avio_get_str(pb, size, str, size + 1); >> > > > + if (n < size) >> > > > + avio_skip(pb, size - n); >> > > >> > > What? Why do you want to skip past data in the input? Size here is just a >> > > random static upper limit of how large an SDP file would be. >> > yes, my fault, don't need to run avio_skip(). For avio_get_str() will return error only >> > buflen <=0, so str is valid anyway. >> > >> > > >> > > This patch totally lacks a motivation of why you want to do this. >> > >> > considering the SDP is strings in file, so I think it's better to use avio_get_str(). >> >> Maybe, but warranting a patch changing things on its own, IMO. >> >> > Also, I'm considering to remove limit of SDP_MAX_SIZE after that. >> >> If you'd defer this patch to a patchset that does that, it might make more >> sense. But I don't see much value in applying it on its own. > > Yes, I consider to add new api which is avio_get_str_len() to get the string len from pb, > before that, I want to hear no objection to use avio_get_str(). then I'll submit a patchset > for the complete change for review. I thought you simply want to read the file to memory. You don't need new API function to do that, simply use avio_read_to_bprint() with INT_MAX upper limit. It will allocate the necessary buffer for you, and you can check if ENOMEM happened with av_bprint_is_complete(), and free the buffer after parsing with av_bprint_finalize(). Regards, Marton
On Wed, Dec 01, 2021 at 06:55:19PM +0100, Marton Balint wrote: > > > On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote: > > > On Wed, Dec 01, 2021 at 05:17:08PM +0200, Martin Storsjö wrote: > > > On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote: > > > > > > > On Wed, Dec 01, 2021 at 03:55:37PM +0200, Martin Storsjö wrote: > > > > > On Wed, 1 Dec 2021, lance.lmwang@gmail.com wrote: > > > > > > > > From: Limin Wang <lance.lmwang@gmail.com> > > > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com> > > > > > > --- > > > > > > libavformat/rtsp.c | 30 ++++++++++++++++++++---------- > > > > > > 1 file changed, 20 insertions(+), 10 deletions(-) > > > > > > > diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c > > > > > > index e6a4993..ec8be8b 100644 > > > > > > --- a/libavformat/rtsp.c > > > > > > +++ b/libavformat/rtsp.c > > > > > > @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name, > > > > > > av_strlcatf(buf, size, ",%s", addrs[i]->addr); > > > > > > } > > > > > > > +static char *read_sdp_str(AVIOContext *pb, int size) > > > > > > +{ > > > > > > + int n; > > > > > > + char *str; > > > > > > + > > > > > > + if (size < 0 || size == INT_MAX) > > > > > > + return NULL; > > > > > > + > > > > > > + str = av_malloc(size + 1); > > > > > > + if (!str) > > > > > > + return NULL; > > > > > > + n = avio_get_str(pb, size, str, size + 1); > > > > > > + if (n < size) > > > > > > + avio_skip(pb, size - n); > > > > > > > What? Why do you want to skip past data in the input? Size > > > here is just a > > > > > random static upper limit of how large an SDP file would be. > > > > yes, my fault, don't need to run avio_skip(). For avio_get_str() will return error only > > > > buflen <=0, so str is valid anyway. > > > > > > > > This patch totally lacks a motivation of why you want to > > > do this. > > > > > considering the SDP is strings in file, so I think it's better > > > to use avio_get_str(). > > > > > > Maybe, but warranting a patch changing things on its own, IMO. > > > > > > > Also, I'm considering to remove limit of SDP_MAX_SIZE after that. > > > > > > If you'd defer this patch to a patchset that does that, it might make more > > > sense. But I don't see much value in applying it on its own. > > > > Yes, I consider to add new api which is avio_get_str_len() to get the > > string len from pb, before that, I want to hear no objection to use > > avio_get_str(). then I'll submit a patchset for the complete change for > > review. > > I thought you simply want to read the file to memory. You don't need new API > function to do that, simply use avio_read_to_bprint() with INT_MAX upper > limit. It will allocate the necessary buffer for you, and you can check if > ENOMEM happened with av_bprint_is_complete(), and free the buffer after > parsing with av_bprint_finalize(). that's cool, I haven't notice avio_read_to_bprint() existing, will try it. > > Regards, > Marton > _______________________________________________ > 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".
diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c index e6a4993..ec8be8b 100644 --- a/libavformat/rtsp.c +++ b/libavformat/rtsp.c @@ -2369,11 +2369,29 @@ static void append_source_addrs(char *buf, int size, const char *name, av_strlcatf(buf, size, ",%s", addrs[i]->addr); } +static char *read_sdp_str(AVIOContext *pb, int size) +{ + int n; + char *str; + + if (size < 0 || size == INT_MAX) + return NULL; + + str = av_malloc(size + 1); + if (!str) + return NULL; + n = avio_get_str(pb, size, str, size + 1); + if (n < size) + avio_skip(pb, size - n); + return str; +} + + static int sdp_read_header(AVFormatContext *s) { RTSPState *rt = s->priv_data; RTSPStream *rtsp_st; - int size, i, err; + int i, err; char *content; char url[MAX_URL_SIZE]; @@ -2386,19 +2404,11 @@ static int sdp_read_header(AVFormatContext *s) rt->lower_transport = RTSP_LOWER_TRANSPORT_CUSTOM; /* read the whole sdp file */ - /* XXX: better loading */ - content = av_malloc(SDP_MAX_SIZE); + content = read_sdp_str(s->pb, SDP_MAX_SIZE - 1); if (!content) { ff_network_close(); return AVERROR(ENOMEM); } - size = avio_read(s->pb, content, SDP_MAX_SIZE - 1); - if (size <= 0) { - av_free(content); - ff_network_close(); - return AVERROR_INVALIDDATA; - } - content[size] ='\0'; err = ff_sdp_parse(s, content); av_freep(&content);