Comment on attachment 249676 [details] [review]
g_setenv() docs updated.

We don't want different behavior on different systems.

Given that passing NULL worked in the past, and some programs apparently depended on this, I think we need to do:

  if (!value)
    {
      g_unsetenv (variable);
      return;
    }

The question is whether we (a) document this behavior, (b) silently accept it without changing the docs, or (c) add a g_warning() before the g_unsetenv().

Created attachment 249729 [details] [review]
g_setenv() with NULL value is equivalent to g_unsetenv()

Did it really work in the past for OSX/BSD or simply nobody ever bothered raising a report here?

Here is the link I found about what I guess is the implemented setenv() in OSX:
http://www.opensource.apple.com/source/Libc/Libc-825.26/stdlib/FreeBSD/setenv.c

I don't really know Apple versioning logics, so I don't know what version of glibc they actually use in any version of OSX, but whatever random version I check in the root source/Libc of the above link, I can see why a NULL value would crash a program (basically they dereference and check its first character before even checking it is not NULL). And that seems to go back to some pretty old code.

Anyway I agree that it would be much nicer and helping the user to fix it on glib side than having only a documentation of good practice. I would still recommend to document this behavior as well, though. I like exhaustiveness and am always annoyed when I wonder what is the behavior of a third party library in extreme cases not written in the doc (so I end up checking the third party code, or testing).

So here is my patch which adds the behavior and document it.

I don't think we should add this behaviour. The g_setenv docs don't document value as 'may be NULL'. If it worked in the past because the platform setenv silently accepts NULL, thats an accident.

Hi,

well personally I have mixed feelings. g_getenv() can indeed return NULL when the environment variable is unset. So if we were to consider g_setenv() like totally mirroring g_getenv(), we would indeed say that setting a variable to NULL actually means unsetting it. Some kind of semantics logics, I'd say.
This is why, in absence of clear documentation of this case, I assumed it would work at first, until users reported crashes on OSX for GIMP. Because it seems "semantically logical".

But in the end, I don't really care as long as this is well documented. If the doc clearly says that you cannot g_setenv() NULL because it may have undefined behavior (even crash on some platform), then it is ok. That comes back to my first patch.

So all what matters is that whatever is chosen, documentation is accurate.

Any news about this one?

I personally don't mind one way or another (basically my first or my second patch), as long as the documentation is consistent with the behavior. :-)

fixed by making it g_return_val_if_fail() everywhere, as Matthias
suggested in comment 3

Hi Dan,

that's good to me but the documentation of g_setenv() should also be updated with information on this behavior in my opinion. This was the whole point of my bug report and my first proposed patch, because currently there is absolutely no documentation about the fact that a NULL @value is forbidden and may even crash the system.

I indeed say it can still crash the system because I remind g_return_val_if_fail() will not necessarily be executed and can therefore not be considered part of the code (this is mostly a developer bug tracing help, as I see it), nor a valid fix of anything in my opinion. Cf. https://developer.gnome.org/glib/stable/glib-Warnings-and-Assertions.html#g-return-val-if-fail

« If G_DISABLE_CHECKS is defined then the check is not performed. You should therefore not depend on any side effects of expr.  »

This is why I reopen this bug to make documentation complete by saying that a NULL @value is forbidden.

Not allowing NULL is the default; it doesn't need to be documented. (Note, for example, that we don't document the fact that @variable can't be NULL.)

See https://bugzilla.gnome.org/show_bug.cgi?id=710582 for fallout.