Christian Couder <christian.couder@xxxxxxxxx> writes: > Using strvec_push() to push `NULL` into a 'strvec' results in a > segfault, because `xstrdup(NULL)` crashes. > > So when an URL is missing from the config, let's push an empty string > instead of `NULL` into the 'strvec' that stores URLs. > > We could have modified strvec_push() to behave like > strvec_push_nodup() and accept `NULL`, but it's not clear that it's > the right thing to do for the strvec API. 'strvec' is a kind of NULL > terminated array that is designed to be compatible with 'argv' > variables used on the command line. So we might want to disallow > pushing any `NULL` in it instead. > > It's also not clear if `xstrdup(NULL)` should crash or BUG or just > return NULL. Yup, the above two paragraphs are irrelevant, I would think. What we could have done may be to ignore (or error out) a configuration entry that lacks URL as an error. After all, isn't this caused by a misconfiguration? How such a misconfiguration is swept under the rug, whether with an empty string or NULL, is secondary, I would think. > For all these reasons, let's just focus on fixing the issue in > "promisor-remote.c" and let's leave improving the strvec API and/or > xstrdup() for a future effort. Absolutely. > While at it let's warn and reject the remote, in the 'KnownUrl' > case, when no URL is advertised by the server or no URL is > configured on the client for a remote name advertised by the server > and configured on the client. This is on par with a warning already > emitted when URLs are different in the same case. Yup.
