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

[Tomas Härdin]

tor 2022-02-03 klockan 18:29 +0100 skrev Mark Gaiser:
> 
> +typedef struct IPFSGatewayContext {
> +    AVClass *class;
> +    URLContext *inner;
> +    char *gateway;

Consider two separate variables. One for AVOption and one for the
dynamically allocated string. Or put the latter on the stack.

> +} IPFSGatewayContext;
> +
> +// A best-effort way to find the IPFS gateway.
> +// Only the most appropiate gateway is set. It's not actually
> requested
> +// (http call) to prevent a potential slowdown in startup. A
> potential timeout
> +// is handled by the HTTP protocol.
> +//
> +// Return codes can be:
> +// 1 : A potential gateway is found and set in c->gateway
> +// -1: The IPFS data folder could not be found
> +// -2: The gateway file could not be found
> +// -3: The gateway file is found but empty
> +// -4: $HOME is empty
> +// -9: Unhandled error

What Michael meant with better return codes is using AVERROR_* :)

> +static int populate_ipfs_gateway(URLContext *h)
> +{
> +    IPFSGatewayContext *c = h->priv_data;
> +    char *ipfs_full_data_folder = NULL;
> +    char *ipfs_gateway_file = NULL;

These can be char[PATH_MAX]

> +    struct stat st;
> +    int stat_ret = 0;
> +    int ret = -9;
> +    FILE *gateway_file = NULL;
> +    char gateway_file_data[1000];

A maximum URL length of 999?

> +
> +    // First, test if there already is a path in c->gateway. If it
> is then it
> +    // was provided as cli arument and should be used. It takes
> precdence.
> +    if (c->gateway != NULL) {
> +        ret = 1;
> +        goto err;
> +    }
> +
> +    // Test $IPFS_GATEWAY.
> +    if (getenv("IPFS_GATEWAY") != NULL) {
> +        av_free(c->gateway);

Useless since c->gateway is NULL

> +
> +        // Stat the folder.
> +        // It should exist in a default IPFS setup when run as local
> user.
> +#ifndef _WIN32
> +        stat_ret = stat(ipfs_full_data_folder, &st);
> +#else
> +        stat_ret = win32_stat(ipfs_full_data_folder, &st);
> +#endif

Again, there is no reason to stat this. Just try opening the gateway
file directly.

> +
> +    // Read a single line (fgets stops at new line mark).
> +    fgets(gateway_file_data, sizeof(gateway_file_data) - 1,
> gateway_file);

This can result in gateway_file_data not being NUL terminated

> +
> +    // Replace first occurence of end of line to \0
> +    gateway_file_data[strcspn(gateway_file_data, "\r\n")] = 0;

What if the file uses \n or no newlines at all?

> +err:
> +    if (gateway_file)
> +        fclose(gateway_file);
> +
> +    av_free(ipfs_full_data_folder);
> +    av_free(ipfs_gateway_file);

This is not cleaning up dynamic allocations of c->gateway

> +// -3: The gateway url part (without the protocol) is too short. We
> expect 3
> +//     characters minimal. So http://aaa would be the bare minimal.

http://1 is valid I think. It means http://0.0.0.1

> +    // Test if the gateway starts with either http:// or https://
> +    // The remainder is stored in url_without_protocol
> +    if (av_stristart(uri, "http://", &url_without_protocol) == 0
> +        && av_stristart(uri, "https://", &url_without_protocol) ==
> 0) {
> +        av_log(h, AV_LOG_ERROR, "The gateway URL didn't start with
> http:// or https:// and is therefore invalid.\n");
> +        ret = -2;
> +        goto err;
> +    }

I guess restricting this to HTTP schemes is OK. Or are there non-HTTP
gateways for this?

> +    if (last_gateway_char != '/') {
> +        c->gateway = av_asprintf("%s/", c->gateway);

Yet another leak

>     // Sanitize the gateway to a format we expect.
> +    if (sanitize_ipfs_gateway(h) < 1)
> +        goto err;

This will return unset ret, thus leaking data from the stack

> +static int ipfs_close(URLContext *h)
> +{
> +    IPFSGatewayContext *c = h->priv_data;

Here is where you'd put any deallocations

The quality of this patch is making me re-affirm what I've already said
viz parsing. bash+sed is superior.

/Tomas