9 ms·

How do you propose making it thread-safe? The real problem here is that `getenv()` was designed around it returning a `char *` into some read-only memory. It's

by DSMan195276 3y ago

How do you propose making it thread-safe? The real problem here is that `getenv()` was designed around it returning a `char *` into some read-only memory. It's a bad API if the backing data can change because the returned pointer is assumed to exist 'forever'. `setenv()` has no way to knowing where those pointers are floating around so there's no way to safely change the environment variables. The best you could do would be to leak memory every time you set new environment variables so that the old pointers don't get invalidated, and that just creates a new problem and reason not to use `setenv()` (that's arguably worse).

cnity 3y ago
Here's my proposal: Introduce a new threadsafe API (`tgetenv` or whatever) which takes _two_ `char *`s, one of which is a return buffer. This leaves allocation as a responsibility of the caller. And then you can leave the existing syscalls as they are (thread unsafe) while having a separate thread safe version.
- DSMan195276 3y ago
  I agree that would be the way to do it, but now we're no longer talking about simply 'fixing' the implementation of the existing API but rather introducing a new function you have to use. `setenv()` would only be safe if your program never uses `getenv()`, and calls to `getenv()` are so numerous and all over the place that for most non-trivial programs it would be hard to ensure they never happen. There's also the rub that `setenv()` is not part of the C standard, it's POSIX. I don't think the C standard would ever introduce `tgetenv()` to fix a problem it doesn't have, so non-POSIX code would have to continue to call `getenv()` since that's all that is available to them.
  - leoh 3y ago
    >you have to use I mean, why not just deprecate the old one; add a warning if it’s used
    - DSMan195276 3y ago
      That doesn't really help you determine whether a given library is using `getenv()` or not. That also requires that things are actually recompiled/updated, which for some C libraries is not that often. There's also the rub that many C programs do not target the latest standard (for a variety of reasons). I didn't realize `getenv_s` was added in C11 (though it's optional), but it doesn't really matter because programs/libraries that target C89 or C99 can't use it anyway.
      - leoh 3y ago
        That’s a good point. I would guess there are ways of doing static analysis to see if a given binary is making getenv() calls though, even if one doesn’t fully grok its source. Maybe some combo of that with sentenv() in your source or something Or do “live” analysis under integration and give a low priority warning But yeah it’s hairy, you’re right
  - josefx 3y ago
    > I don't think the C standard would ever introduce `tgetenv()` to fix a problem it doesn't have The C standard has no problem acknowledging that getenv is subject to data races for most of its implementations. As far as I can tell that part was even added at the same time as threading support.
    - DSMan195276 3y ago
      Well actually I'll have to eat my words on that one - I didn't catch that Annex K in C11 includes `getenv_s` (even if it is optional).
- salawat 3y ago
  GNU convention is <funcname>_r for a reentrant version of a non reentrant function. I'm in the process of working on a tool in C at the moment, so for once I actually have some context on what's being grumped about here!
- JohnFen 3y ago
  Entirely this. It works so well that I've seen this in various utility libraries for decades.
  - cnity 3y ago
    Yeah. Most of my code these days works like this, to the point where almost all of my allocation happens roughly in my main entry-point. Or I use arena allocators and pass those into my utilities. But there's basically never a `realloc` or `malloc` call deep in my code anymore.
forrestthewoods 3y ago
> to safely change the environment variables. The best you could do would be to leak memory every time you set new environment variables so that the old pointers don't get invalidated, and that just creates a new problem and reason not to use `setenv()` (that's arguably worse). Arguably worse? My goodness no. This is a rare edge case that most programs don’t encounter. Option 1 is to crash and explode and die. Option 2 is to leak tens of bytes. Leaking tens of bytes is for sure NOT worse than crashing.
- marcosdumay 3y ago
  > Leaking tens of bytes is for sure NOT worse than crashing. I do really disagree here. The answer is not clear at all. But then, you are mischaracterizing the problem. The issue is not with crashing, you can get plain bad data too, and this is clearly worse than both leaking memory and crashing. Also, the GP is mischaracterizing the options. You don't need to leave the old values around, you can just copy them into userspace memory.
- DSMan195276 3y ago
  My reasoning is simple - the issues here can be avoided if you're careful about how you use `setenv()` and `getenv()`, which many programs already are. The memory leak in contrast would never be avoidable regardless of how you use it.
  - forrestthewoods 3y ago
    The problem with “be careful” is that libraries often want to use the very unsafe API and there is no standard mechanism to expose safety. It’s fundamentally a bad API design. It could be good. But it is not. There’s a reason this problem comes up on HN once or two a year. And don’t even get me started about printf grabbing a mutex for a stupid locale…
- none_to_remain 3y ago
  You could even put a note in the man page that `setenv()` will leak memory. Then ten or twenty years from now there will be a blog post about how a currently trendy language/runtime can be manipulated into looping over `setenv()` zillions of times and OOM'ing, and comments about how no one can possibly be expected to read the man page for this horrible footgun, and it's wrong to expect developers to have any idea about what they're doing, give a shit, or pay attention at all.

cnity 3y ago

Here's my proposal: Introduce a new threadsafe API (`tgetenv` or whatever) which takes _two_ `char *`s, one of which is a return buffer. This leaves allocation as a responsibility of the caller. And then you can leave the existing syscalls as they are (thread unsafe) while having a separate thread safe version.

DSMan195276 3y ago
I agree that would be the way to do it, but now we're no longer talking about simply 'fixing' the implementation of the existing API but rather introducing a new function you have to use. `setenv()` would only be safe if your program never uses `getenv()`, and calls to `getenv()` are so numerous and all over the place that for most non-trivial programs it would be hard to ensure they never happen. There's also the rub that `setenv()` is not part of the C standard, it's POSIX. I don't think the C standard would ever introduce `tgetenv()` to fix a problem it doesn't have, so non-POSIX code would have to continue to call `getenv()` since that's all that is available to them.
- leoh 3y ago
  >you have to use I mean, why not just deprecate the old one; add a warning if it’s used
  - DSMan195276 3y ago
    That doesn't really help you determine whether a given library is using `getenv()` or not. That also requires that things are actually recompiled/updated, which for some C libraries is not that often. There's also the rub that many C programs do not target the latest standard (for a variety of reasons). I didn't realize `getenv_s` was added in C11 (though it's optional), but it doesn't really matter because programs/libraries that target C89 or C99 can't use it anyway.
    - leoh 3y ago
      That’s a good point. I would guess there are ways of doing static analysis to see if a given binary is making getenv() calls though, even if one doesn’t fully grok its source. Maybe some combo of that with sentenv() in your source or something Or do “live” analysis under integration and give a low priority warning But yeah it’s hairy, you’re right
- josefx 3y ago
  > I don't think the C standard would ever introduce `tgetenv()` to fix a problem it doesn't have The C standard has no problem acknowledging that getenv is subject to data races for most of its implementations. As far as I can tell that part was even added at the same time as threading support.
  - DSMan195276 3y ago
    Well actually I'll have to eat my words on that one - I didn't catch that Annex K in C11 includes `getenv_s` (even if it is optional).
salawat 3y ago
GNU convention is <funcname>_r for a reentrant version of a non reentrant function. I'm in the process of working on a tool in C at the moment, so for once I actually have some context on what's being grumped about here!
JohnFen 3y ago
Entirely this. It works so well that I've seen this in various utility libraries for decades.
- cnity 3y ago
  Yeah. Most of my code these days works like this, to the point where almost all of my allocation happens roughly in my main entry-point. Or I use arena allocators and pass those into my utilities. But there's basically never a `realloc` or `malloc` call deep in my code anymore.

forrestthewoods 3y ago

> to safely change the environment variables. The best you could do would be to leak memory every time you set new environment variables so that the old pointers don't get invalidated, and that just creates a new problem and reason not to use `setenv()` (that's arguably worse). Arguably worse? My goodness no. This is a rare edge case that most programs don’t encounter. Option 1 is to crash and explode and die. Option 2 is to leak tens of bytes. Leaking tens of bytes is for sure NOT worse than crashing.

marcosdumay 3y ago
> Leaking tens of bytes is for sure NOT worse than crashing. I do really disagree here. The answer is not clear at all. But then, you are mischaracterizing the problem. The issue is not with crashing, you can get plain bad data too, and this is clearly worse than both leaking memory and crashing. Also, the GP is mischaracterizing the options. You don't need to leave the old values around, you can just copy them into userspace memory.
DSMan195276 3y ago
My reasoning is simple - the issues here can be avoided if you're careful about how you use `setenv()` and `getenv()`, which many programs already are. The memory leak in contrast would never be avoidable regardless of how you use it.
- forrestthewoods 3y ago
  The problem with “be careful” is that libraries often want to use the very unsafe API and there is no standard mechanism to expose safety. It’s fundamentally a bad API design. It could be good. But it is not. There’s a reason this problem comes up on HN once or two a year. And don’t even get me started about printf grabbing a mutex for a stupid locale…
none_to_remain 3y ago
You could even put a note in the man page that `setenv()` will leak memory. Then ten or twenty years from now there will be a blog post about how a currently trendy language/runtime can be manipulated into looping over `setenv()` zillions of times and OOM'ing, and comments about how no one can possibly be expected to read the man page for this horrible footgun, and it's wrong to expect developers to have any idea about what they're doing, give a shit, or pay attention at all.