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