Thilo Borgmann: > Am 14.01.22 um 19:05 schrieb Andreas Rheinhardt: >> Thilo Borgmann: >>> Am 14.01.22 um 14:17 schrieb "zhilizhao(赵志立)": >>>> >>>> >>>>> On Jan 14, 2022, at 8:14 PM, Thilo Borgmann <thilo.borgm...@mail.de> >>>>> wrote: >>>>> >>>>> Am 06.01.22 um 12:27 schrieb Thilo Borgmann: >>>>>> Am 03.01.22 um 16:22 schrieb Thilo Borgmann: >>>>>>> Am 29.12.21 um 12:46 schrieb Nicolas George: >>>>>>>> "zhilizhao(赵志立)" (12021-12-29): >>>>>>>>> How about add a restriction like this: >>>>>>>>> >>>>>>>>> if (format.endsWith(“%S")) >>>>>>>>> enable the feature >>>>>>>>> else >>>>>>>>> warning message >>>>>>>>> >>>>>>>>> It’s a useful feature, it shouldn't create unexpected results, but >>>>>>>>> doesn’t need to support every use case. >>>>>>>> >>>>>>>> I would not oppose it, but I find it inelegant, especially because it >>>>>>>> requires a different expansion function, localtime_ms instead of >>>>>>>> localtime. >>>>>>>> >>>>>>>> What about this: with the original function "localtime", if the format >>>>>>>> ends in "%3N", then append the millisecond. It can later be expanded to >>>>>>>> support %xN at any place in the format for any value of x. >>>>>>> >>>>>>> I think best will be to scan the format string for %S and extend it >>>>>>> there with .ms part before expanding the rest of it, not? Shouldn't be >>>>>>> too expensive for the filter. >>>>>>> >>>>>>> Just need to find time to actually implement it. >>>>>> >>>>>> Like v5 as attached. >>>> >>>> >>>>> + if (tag == 'M' || tag == 'm') { >>>>> + char *seconds = av_stristr(fmt, "%S"); >>>>> + if (seconds) { >>>>> + seconds += 2; >>>>> + int len = seconds - fmt + 1; >>>>> + char *tmp = av_malloc(len); >>>>> + av_strlcpy(tmp, fmt, len); >>>> >>>> Firstly, mixed variable declaration and statements. >>>> >>>> Secondly, I think you don’t need ’tmp’, something like >>>> >>>> av_asprintf(“%.*s.%03d%s", len, fmt, (int)(unow % 1000000) / 1000, >>>> seconds); >>> >>> You know your printf format-string :) >>> >>> Thanks, v6 attached. >>> -Thilo >>> >>> >> >>> >>> + int len = seconds + 2 - fmt; >>> + char *fmt_new = av_asprintf("%.*s.%03d%s", len, fmt, >>> (int)(unow % 1000000) / 1000, seconds + 2); >>> + av_bprint_strftime(bp, fmt_new, &tm); >>> + return 0; >>> + } >> >> I see an unchecked allocation and a leak. > > Ok fmt_new might be null, where is the leak? >
Where is fmt_new freed? > >> And it seems you are using a >> format string that comes from the user. This is undefined behaviour if >> this string contains an invalid conversion specifier. > > I think that was unfortunately true before the patch as well, was it not? Seems so. > And if true or not, do we have something in place to check a user string? > Afaik no. - Andreas _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".