Re: [FFmpeg-devel] [PATCH 3/4] fftools/textformat: infer type in mermaid_print_value

["softworkz ." <softworkz-at-hotmail.com@ffmpeg.org>]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Marvin Scholz
> Sent: Donnerstag, 12. Juni 2025 13:34
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 3/4] fftools/textformat: infer
> type in mermaid_print_value
> 
> 
> 
> On 12 Jun 2025, at 4:19, softworkz . wrote:
> 
> > Hi Marvin,
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> >> Marvin Scholz
> >> Sent: Mittwoch, 11. Juni 2025 21:57
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: [FFmpeg-devel] [PATCH 3/4] fftools/textformat: infer
> type in
> >> mermaid_print_value
> >>
> >> Instead of the caller having to indicate if it is passing an
> >> integer, just use the fact that str is NULL when an integer is
> >> to be printed.
> >> ---
> >>  fftools/textformat/tf_mermaid.c | 12 ++++++------
> >>  1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/fftools/textformat/tf_mermaid.c
> >> b/fftools/textformat/tf_mermaid.c
> >> index dbe489a7a7..da371c8fff 100644
> >> --- a/fftools/textformat/tf_mermaid.c
> >> +++ b/fftools/textformat/tf_mermaid.c
> >> @@ -547,7 +547,7 @@ static void
> >> mermaid_print_section_footer(AVTextFormatContext *tfc)
> >>  }
> >>
> >>  static void mermaid_print_value(AVTextFormatContext *tfc, const
> char
> >> *key,
> >> -                                const char *str, int64_t num,
> const
> >> int is_int)
> >> +                                const char *str, int64_t num)
> >>  {
> >>      MermaidContext *mmc = tfc->priv;
> >>      const AVTextFormatSection *section = tf_get_section(tfc,
> tfc-
> >>> level);
> >> @@ -587,7 +587,7 @@ static void
> >> mermaid_print_value(AVTextFormatContext *tfc, const char *key,
> >>          switch (mmc->diagram_config->diagram_type) {
> >>          case AV_DIAGRAMTYPE_GRAPH:
> >>
> >> -            if (is_int) {
> >> +            if (!str) {
> >
> > This is not right - it would print 0 when the str is NULL.
> > See tf_xml, it uses the same pattern (is_int).
> >
> 
> So what do you expect to happen when it is NULL?
> 
> From what I have seen passing NULL for str would
> make these functions actually UB as passing NULL
> for a string to printf like functions is not defined,
> unless I missed this being handled explicitly somewhere.

Hi Marvin,

you are surely right to point at this. There is still an ongoing
discussion about parameter validation (with Stefano Sabatini) and 
I have a patchset for it that isn't submitted yet. This adds 
parameter validation in several ways and to several functions.
For example, for the corresponding function in tf_xml.c, it 
adds this to the function at the start:

static void xml_print_value(AVTextFormatContext *wctx, const char *key,
                            const char *str, int64_t num, const int is_int)
{
    AVBPrint buf;
    XMLContext *xml = wctx->priv;
    const struct AVTextFormatSection *section = tf_get_section(wctx, wctx->level);
    
    if (!section)
        return;
    
    if (!tf_validate_string_param(key, "key", wctx) ||
        (!is_int && !tf_validate_string_param(str, "value", wctx))) {
        return;
    }
....

This kind of "plot hole" has existed ever since in the original code.
The merged patchset was about generalizing and refactoring code but 
not about making functional changes to the APIs. I made a cut at this
point to avoid the patchset to become even more excessive. 

I will submit the next step about parameter validation in a while,
when I find some time to revisit this topic.

Thanks,
sw

_______________________________________________
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".