Re: [FFmpeg-devel] [PATCH v2 1/1] avformat: Add IPFS protocol support.

["Tomas Härdin" <tjoppen@acc.umu.se>]

ons 2022-02-02 klockan 14:56 +0100 skrev Mark Gaiser:
> On Wed, Feb 2, 2022 at 2:21 PM Tomas Härdin <tjoppen@acc.umu.se>
> wrote:
> 
> > tis 2022-02-01 klockan 22:58 +0100 skrev Mark Gaiser:
> > 
> > > 
> > > +typedef struct Context {
> > > +    AVClass *class;
> > > +    URLContext *inner;
> > > +    char *gateway;
> > 
> > Is there not a maximum length that an HTTP URL can be? At least
> > without
> > query parameters. That way you avoid dynamic allocations. You'd
> > have to
> > separate the AVOption from such a buffer in that case, but I think
> > you
> > have to anyway.
> > 
> 
> Could you provide more information on that? Or an example of what you
> mean
> exactly?
> As far as i know there is no hard limit though it's very much advised
> to
> not go above 2048 characters.

You could at least separate the variable that is set by the AVOption
stuff from the buffer that's produced by the URL parsing stuff.

Since you call ffurl_open_whitelist() immediately you could just use
alloca() to allocate memory on the stack. That way you don't have to
worry about deallocation at all *and* you can have it be arbitrarily
large.

If URLs have a spec'd maximum length then a buffer of constant size on
the stack would work.

> 
> > 
> > > +    if (!ipfs_full_data_folder) {
> > > +        av_log(h, AV_LOG_DEBUG, "$IPFS_PATH is empty.\n");
> > > +
> > > +        // Try via the home folder.
> > > +        home_folder = getenv("HOME");
> > > +        ipfs_full_data_folder = av_asprintf("%s/.ipfs/",
> > > home_folder);
> > 
> > Memory leak. This applies to most if not all av_asprintf() calls.
> > 
> 
> Is there an advised way to neatly clean that up?
> Sure, I can add a bunch of av_free calls to clean it up. But there
> are
> places where it's not as straightforward like where the av_asprintf
> was
> done in an if statement. How do I maintain the knowledge that
> av_asprintf
> was used to call av_free later?

goto as the others pointed out


> 
> > 
> > > +static int translate_ipfs_to_http(URLContext *h, const char
> > > *uri,
> > > int flags, AVDictionary **options)
> > > +{
> > > +    const char *ipfs_cid;
> > > +    const char *protocol_path_suffix = "ipfs/";
> > > +    char *fulluri;
> > > +    int ret;
> > > +    Context *c = h->priv_data;
> > > +    int is_ipfs = (av_strstart(uri, "ipfs://", &ipfs_cid) ||
> > > av_strstart(uri, "ipfs:", &ipfs_cid));
> > > +    int is_ipns = (av_strstart(uri, "ipns://", &ipfs_cid) ||
> > > av_strstart(uri, "ipns:", &ipfs_cid));
> > 
> > https://docs.ipfs.io/concepts/ipfs-gateway/ claims ipfs:// is the
> > canonical form. No mentioned is made of any ipfs:{CID} form.
> > Incorrect
> > URLs should be rejected, not silently patched.
> > 
> 
> I'd like to make a decision here. This current logic (ipfs:// and
> ipfs:,
> same for ipns) is inspired by other protocols that ffmpeg supported.
> I
> simply copied how they work to be consistent.
> Do i:
> 1. keep it as is and be consistent with the rest?
> 2. only allow ipfs:// and ipns://?
> 

Which protocols? This laxness in lavf is worrisome. It breeds laxness
in other projects.

/Tomas

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