@stewid: let me know if you want to be named in a Reported-by line.

I’m interested in your analysis of this problem. When are we calling this function with a null buffer? Why should we change this and not the caller?

As determined in the referenced issue, there are two callers, both in parse.c, both carrying out operations on a git_parse_ctx. In both cases, git__linenlen() is used for determining and setting the length (field line_len) of the next line (pointed to by field line of a git_parse_ctx). line may be NULL, e.g. if:

• the parse context is be initialized with NULL for the buffer,
• 0 is passed for the buffer-length to the initialization function or
• the context is reset using git_parse_ctx_clear().

Based on this, I considered three options:

1. Enforcing field line inside git_parse_ctx to be non-null: having NULL mean "empty buffer" is quite common, but I suspect that the context is actually always initialized with some non-null buffer which would, in turn, result in a non-null line. However, I did not check. Also, changing the semantics so that line is set to a pointer to an empty string would mean introducing multiple checks, extra code for git_parse_ctx_clear() and, most notably, would introduce an invariant rather uncommon (in C) for this type of object.
2. Adding additional checks to the call-site: every call-site is affected, so every (existing) call-site would need some additional check. In the case of git_parse_advance_line(), this could be an early return, which would provide the additional benefit of the line_num field not being incremented. However, I assume that this does not really matter, since the line-number may be considered "undefined" anyway for a null-buffer (it's not nice, but I doubt anybody cares).
3. Adding an additional check to git__linenlen() (as proposed in this PR): this required adding only a single check, rather than a check for every single call-site. This makes the function "safe" for the situations of interest, both for the existing and possible future uses of that function.

If desired, I can either change this PR or file an alternative PR implementing the 2nd of those approaches rather then the 3rd.

One of the AppVeyor jobs failed due to network problems. It should be sufficient to simply restart the job.

I've restarted the AppVeyor build

Thanks for the analysis, @neithernut. The reason I asked is because few (none?) of our other string helper functions do a null check like this. (I didn't do an exhaustive analysis, I just quickly skimmed util.c.) I'd like to take a moment to do a little more digging to ensure that we don't change a function to be very dissimilar than the rest of the internal library when it comes to its preconditions.

While most (all?) other string functions don't do NULL checks, I find the semantics in this case to be rather self-evident: if you've got no string, the line length is clearly "0". On the other hand strlen(3P) exhibits undefined behaviour in case its parameter is NULL, which would be a case against including the NULL check in git__linelen. Also: even though we'd be safe wrt git_linelen, it might still be that we pass the NULL string to other functions afterwards, as @ethomson said.

So this all speaks against my previous thinking. So maybe we should make sure that a parser context is never being initialized with a NULL pointer?

So this all speaks against my previous thinking. So maybe we should make sure that a parser context is never being initialized with a NULL pointer?

That makes sense to me.

In this case, I'll prepare patches implementing approach 2 (changing git_parse_*). Probably over the weekend.