Fix: sapi_getenv: value should be initialized and not used so

[hwde]

No description provided.

[Girgias]

It probably makes more sense to switch all the logic to use guards returning NULL instead of the weird nesting we currently have which would then make it clear that value is not uninitialized.

[Girgias]

The changes made are the opposite of what I was asking, I'm asking for only early returns via if conditions which are easier to read than nesting if conditions

[hwde]

I don't think that this would be easier to read (btw.: isn't the call to strlen() in WIN32 superfluous ... possibly it has been inserted for performance reasons):

SAPI_API char *sapi_getenv(const char *name, size_t name_len)
{
	char *value = NULL, *tmp;
	
	if (!strncasecmp(name, "HTTP_PROXY", name_len)) {
		/* Ugly fix for HTTP_PROXY issue, see bug #72573 */
		return NULL;
	}
	if (!sapi_module.getenv) {
		return NULL;
	}
	tmp = sapi_module.getenv(name, name_len);
	if (!tmp) {
		return NULL;
	}
	value = estrdup(tmp);
#ifdef PHP_WIN32
	if (strlen(sapi_module.name) == sizeof("cgi-fcgi") - 1 && !strcmp(sapi_module.name, "cgi-fcgi")) {
		/* XXX more modules to go, if needed. */
		free(tmp);
	}
#endif
	if (value && sapi_module.input_filter) {
		sapi_module.input_filter(PARSE_STRING, name, &value, strlen(value), NULL);
	}
	return value;
}

[cmb69]

isn't the call to strlen() in WIN32 superfluous ... possibly it has been inserted for performance reasons

Yeah, that looks like a dubious performance optimization.

[Girgias]

I don't think that this would be easier to read (btw.: isn't the call to strlen() in WIN32 superfluous ... possibly it has been inserted for performance reasons):

SAPI_API char *sapi_getenv(const char *name, size_t name_len)
{
	char *value = NULL, *tmp;
	
	if (!strncasecmp(name, "HTTP_PROXY", name_len)) {
		/* Ugly fix for HTTP_PROXY issue, see bug #72573 */
		return NULL;
	}
	if (!sapi_module.getenv) {
		return NULL;
	}
	tmp = sapi_module.getenv(name, name_len);
	if (!tmp) {
		return NULL;
	}
	value = estrdup(tmp);
#ifdef PHP_WIN32
	if (strlen(sapi_module.name) == sizeof("cgi-fcgi") - 1 && !strcmp(sapi_module.name, "cgi-fcgi")) {
		/* XXX more modules to go, if needed. */
		free(tmp);
	}
#endif
	if (value && sapi_module.input_filter) {
		sapi_module.input_filter(PARSE_STRING, name, &value, strlen(value), NULL);
	}
	return value;
}

You don't need to check that value is set, as if a function relying on emalloc cannot allocate memory the engine bails out and we do not check for those. You can add an assertion that value is not NULL.

IMHO nested ifs make understanding the control flow a PITA.
And rearanging the ifs makes it pretty obvious that the first if should be

if (!sapi_module.getenv) {
	return NULL;
}

Instead of the string comparison.