| Message ID | 1492696974-38350-1-git-send-email-derek.buitenhuis@gmail.com |
|---|---|
| State | Accepted |
| Commit | 8e6b9ef4733be91b32c8b7becd95124340b92334 |
| Headers | show |
On Thu, Apr 20, 2017 at 03:02:54PM +0100, Derek Buitenhuis wrote: > The original author apparently never read the documentation for snprintf, > or even tested that the output was correct. This statement does not belong in a commit message. Its insulting to whoever the author is and may not even be true. [...]
On 4/20/2017 3:58 PM, Michael Niedermayer wrote: > On Thu, Apr 20, 2017 at 03:02:54PM +0100, Derek Buitenhuis wrote: >> The original author apparently never read the documentation for snprintf, >> or even tested that the output was correct. > This statement does not belong in a commit message. > Its insulting to whoever the author is and may not even be true. Removed. However it /definitely/ was not tested properly. New message: Output was apparently not tested for correctness. Passing overlapping memory to snprintf causes undefined behavior, and usually resulted in only the very last timestamp being written to metadata, and not a list at all. - Derek
On 4/20/2017 3:02 PM, Derek Buitenhuis wrote: > --- > libavformat/matroskadec.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) Ping. - Derek
On Sun, 23 Apr 2017 14:09:06 +0100 Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > On 4/20/2017 3:02 PM, Derek Buitenhuis wrote: > > --- > > libavformat/matroskadec.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > Ping. I don't think any active developer cares about it. Considering it was broken, nobody nobody did. So just push it. On a quick look, this seems slightly questionable (why the ret==20), but probably ok. Possibly you could CC the original author if you want to have more eyes on it.
On 4/23/2017 4:28 PM, wm4 wrote: > Possibly you could CC the original author if you want to have more eyes > on it. I CC'd him on the other patch already, but I'll loop him in here. - Derek
On Thu, Apr 20, 2017 at 7:02 AM, Derek Buitenhuis <derek.buitenhuis@gmail.com> wrote: > The original author apparently never read the documentation for snprintf, > or even tested that the output was correct. I was the original author and i am sorry about the mistake. > Passing overlapping memory > to snprintf causes undefined behavior, and usually resulted in only the > very last timestamp being written to metadata, and not a list at all. > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > libavformat/matroskadec.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c > index 9adca8d..320d8bf 100644 > --- a/libavformat/matroskadec.c > +++ b/libavformat/matroskadec.c > @@ -3823,6 +3823,7 @@ static int webm_dash_manifest_cues(AVFormatContext *s) > char *buf; > int64_t cues_start = -1, cues_end = -1, before_pos, bandwidth; > int i; > + int end = 0; > > // determine cues start and end positions > for (i = 0; i < seekhead_list->nb_elem; i++) > @@ -3868,10 +3869,17 @@ static int webm_dash_manifest_cues(AVFormatContext *s) > if (!buf) return -1; > strcpy(buf, ""); > for (i = 0; i < s->streams[0]->nb_index_entries; i++) { > - snprintf(buf, (i + 1) * 20 * sizeof(char), > - "%s%" PRId64, buf, s->streams[0]->index_entries[i].timestamp); > - if (i != s->streams[0]->nb_index_entries - 1) > + int ret = snprintf(buf + end, 20 * sizeof(char), > + "%" PRId64, s->streams[0]->index_entries[i].timestamp); > + if (ret <= 0 || (ret == 20 && i == s->streams[0]->nb_index_entries - 1)) { Given that you are trying to fix an issue here, can you please add a comment as to what this if condition is doing? (why ret == 20 for example). But up to you though. > + av_log(s, AV_LOG_ERROR, "timestamp too long.\n"); > + return AVERROR_INVALIDDATA; > + } > + end += ret; > + if (i != s->streams[0]->nb_index_entries - 1) { > strncat(buf, ",", sizeof(char)); > + end++; > + } > } > av_dict_set(&s->streams[0]->metadata, CUE_TIMESTAMPS, buf, 0); > av_free(buf); > -- > 1.8.3.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index 9adca8d..320d8bf 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -3823,6 +3823,7 @@ static int webm_dash_manifest_cues(AVFormatContext *s) char *buf; int64_t cues_start = -1, cues_end = -1, before_pos, bandwidth; int i; + int end = 0; // determine cues start and end positions for (i = 0; i < seekhead_list->nb_elem; i++) @@ -3868,10 +3869,17 @@ static int webm_dash_manifest_cues(AVFormatContext *s) if (!buf) return -1; strcpy(buf, ""); for (i = 0; i < s->streams[0]->nb_index_entries; i++) { - snprintf(buf, (i + 1) * 20 * sizeof(char), - "%s%" PRId64, buf, s->streams[0]->index_entries[i].timestamp); - if (i != s->streams[0]->nb_index_entries - 1) + int ret = snprintf(buf + end, 20 * sizeof(char), + "%" PRId64, s->streams[0]->index_entries[i].timestamp); + if (ret <= 0 || (ret == 20 && i == s->streams[0]->nb_index_entries - 1)) { + av_log(s, AV_LOG_ERROR, "timestamp too long.\n"); + return AVERROR_INVALIDDATA; + } + end += ret; + if (i != s->streams[0]->nb_index_entries - 1) { strncat(buf, ",", sizeof(char)); + end++; + } } av_dict_set(&s->streams[0]->metadata, CUE_TIMESTAMPS, buf, 0); av_free(buf);
The original author apparently never read the documentation for snprintf, or even tested that the output was correct. Passing overlapping memory to snprintf causes undefined behavior, and usually resulted in only the very last timestamp being written to metadata, and not a list at all. Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> --- libavformat/matroskadec.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)