Hi guys, My first post here, hope I do things right. The following code snippet just exits the program silently rather than generating SIGBUS as you would expect it to. I don't think what doing that is appropriate. clang *does* generate SIGBUS, and although the code below is, strictly speaking, UB, I don't think gcc should behave as it does, for obvious reasons. OK, here's the code: #include <iostream> int main() { char *p = (char *) nullptr; std::cout << "Watch the " << p << "birdie" << std::endl; return 0; } And here's the output (note: no SIGBUS): Watch the Live demo here (where you can also try running it with clang) https://wandbox.org/permlink/M1S74HqjT1HvLtr0 Sounds like an easy fix. Thanks.
The iostream inserters for char* require the pointer be non-null (and valid), so their behavior is undefined otherwise. Libstdc++ detects the null pointer and sets badbit in response which has the effect of discarding all subsequent output sent to the stream. If Clang/libc++ fails with a SEGV that's most likely because libc++ doesn't have this feature. FWIW, I don't think think detecting null pointers like this in the library is a useful feature. They should be treated the same as any other invalid pointer: i.e., let GCC decide what to do, which may be to issue a warning when it can detect the pointer is null (and either let the code SEGV or drop the dereference).
Hi Martin, Thanks very much for your prompt reply, and I completely agree with your viewpoint. I therefore hereby request that libstc++ stops behaving like that and just lets the SIGSEGV happen. The current behaviour is very unfriendly.
Arguably crashing the program is more unfriendly. We could add the nonnull attribute to the relevant ostream members, or add assertions that can be optionally enabled, but I'm not convinced that crashing is necessarily an improvement.
@Johnathon Crashing the program is the right thing to do, because it means that the developer (or the test department) will get to find out about the problem before the customer does. Does that help you see why I am pushing for it?
*** Bug 86129 has been marked as a duplicate of this bug. ***
And they don't test for iostream operations failing? The program has undefined behaviour, i.e. a bug. Whether it's better to identify that and treat it as a corrupt stream state (setting badbit, and optionally throwing an exception) or crash the program depends on the program. There's no single choice that's correct for all programs.
I don't know the original reason for handling null pointers here, but it's consistent with glibc's printf which prints "(null)" when a null pointer is provided for a %s specifier. Removing this longstanding behaviour now could break an unnknown number of programs which are (impicitly or explicitly) relying on this behaviour of libstdc++. It's not at all clear to me that crashing is preferable.
Thanks for your comments. I can see there are two sides to this. I was in the middle of composing the tract below. I'll include that anyway because it took me ages to type. There's a bit at the end about people who might get bitten if you do make this change, but a plausible alternative to crashing the program might be to (a) print "(null)", and (b) leave the badbit alone. I think I'd settle for that, I actually quite like the way the printf family behaves. My view of the badbit, BTW, is that it's not there to catch programming errors - it's there to report adverse events that come up at runtime (e.g. cannot open file). I expand on that below. ----- The code I posted was by way of example. I didn't realise at the time what was causing the behaviour I was observing - this whole thing came up on StackOverflow and you can see the thread here (please excuse me calling gcc 'naughty', I didn't have the full picture then): https://stackoverflow.com/questions/50696746/c-template-class-instance-issue I guess my point is that the way things are implemented currently is likely to lead to subtle, undetected bugs. Not everyone checks for errors properly, and not everyone (and that included me until just now) knows that if you want an iostream to throw exceptions you have to tell it so. Result? People innocently write code like that shown on in that thread and think they have done all they need to. But they haven't. So I see the current behaviour as being the worst of all worlds. After all, ostream would likely crash if the pointer was invalid, so what's so special about NULL? They are both programming errors, and not the sort of situation you want to have to write extra code to handle explicitly. Throwing an exception should be reserved for some external influence on the stream (such as a remote file becoming inaccessible due to a network error, say), which you *do* need to handle in your code. To put it another way, what are you going to do in your exception handler if you get a nullptr exception (whatever that is - is there even one?). Display "nullptr encountered, operation aborted" to the user and bail out? That's not going to impress your customer base. The reason I like bringing the program down is that it provides an opportunity to generate a stack trace and post it home somehow. But hopefully, the coding error is discovered before the code even gets out into the field, if you do this. [Additional note: I can see if you change the current behaviour you might get a bit of flak from people whose programs "used to work fine". Well, they didn't, and they should be grateful to you for alerting them to that fact.]
(In reply to Paul Sanders from comment #8) > Thanks for your comments. I can see there are two sides to this. > > I was in the middle of composing the tract below. I'll include that anyway > because it took me ages to type. There's a bit at the end about people who > might get bitten if you do make this change, but a plausible alternative to > crashing the program might be to (a) print "(null)", and (b) leave the > badbit alone. I think I'd settle for that, I actually quite like the way > the printf family behaves. That would be another valid outcome of undefined behaviour, but would make it much harder to detect the problem. Setting badbit makes it possible to detect and take action (including calling std::abort() or throwing an exception, if you want to). > My view of the badbit, BTW, is that it's not there to catch programming > errors - it's there to report adverse events that come up at runtime (e.g. > cannot open file). I expand on that below. Failure to open a file sets failbit, not badbit. > After all, ostream would likely crash if the pointer was invalid, so what's > so special about NULL? It's detectable. > [Additional note: I can see if you change the current behaviour you might > get a bit of flak from people whose programs "used to work fine". Well, > they didn't, and they should be grateful to you for alerting them to that > fact.] The program did work fine though, and anybody can verify that it does so intentionally, not by accident: https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libstdc%2B%2B-v3/include/bits/ostream.tcc;h=e02ba55a6262b96ead90595e1aad56119a965844;hb=HEAD#l323 As the standard says: Permissible undefined behavior ranges from ignoring the situation completely with unpredictable results, to behaving during translation or program execution in a documented manner characteristic of the environment (with or without the issuance of a diagnostic message), to terminating a translation or execution (with the issuance of a diagnostic message).
As a data point, calling printf ("%s", p) does lead to a segfault in Glibc for a null p because GCC turns the call into puts(p) which doesn't have the same feature (see https://sourceware.org/bugzilla/show_bug.cgi?id=5618 for the background). I think most users prefer invalid uses of pointers to fail loudly so they can be caught early. Few users expect output functions to fail, and even fewer bother to check for failures when writing to standard streams. Besides the inserter without the test resulting in more efficient code and GCC emitting a warning when a null pointer is passed to it (it doesn't now even though it should because of the strlen call), leaving it to the compiler to deal with can also lead to better code downstream thanks to -fdelete-null-pointer-checks.
> I think most users prefer invalid uses of pointers to fail loudly so they can be caught early. Few users expect output functions to fail, and even fewer bother to check for failures when writing to standard streams. Yes, that's very much my own personal preference, but as I dig into this more I can see there are problems with doing what I'm asking for. > Removing this longstanding behaviour now could break an unknown number of programs which are (implicitly or explicitly) relying on this behaviour of libstdc++. I think they'd have to be relying on it explicitly (i.e. they turn on badbit exceptions or check explicitly for badbit). Otherwise, they have a bug. Question is, how many are there out there? I doubt many are testing badbit. Not for this reason, anyway. There is however, some info at [cppreference](http://en.cppreference.com/w/cpp/io/ios_base/iostate) which supports your view. Passing in quite a few types as nullptr sets badbit (too many, IMO), but `char *` is not mentioned explicitly. So I suppose it's reasonable for people to expect that _all_ null pointers should be treated in the same way. It's a shame anyone on the standards committee ever thought that testing for null pointers in this way was useful but I suppose we're stuck with it now. Personally, I think it was a big mistake. > Failure to open a file sets failbit, not badbit. Noted, thank you, I'm not very familiar with iostreams. Forget that bit then. > The program did work fine though, and anybody can verify that it does so intentionally, not by accident. Nice link, thank you, but that doesn't change the basic argument that many programs will not be working quite right because they accidentally passed in a null pointer and didn't expect the resulting behaviour of `ostream`. Still, I might in the end have to say here, sadly: c'est la vie.
Sorry, I posted that in a bit of a rush. I took a proper look and the null pointers that set badbit actually make excellent sense. So I'll suggest a way out of the backwards compatibility conundrum when `ostream::operator<<` is passed a null pointer (of any sort). The suggestion I have is to test for a null pointer _only_ if the stream is set up to throw badbit exceptions Otherwise, just blindly dereference the pointer and crash. The rationale for this is that people who have set up an exception handler for badbit _might_ just have done so to catch null pointers. I think it's unlikely but it's possible. Those who haven't are probably getting away with something they shouldn't. Any interest?
(In reply to Martin Sebor from comment #10) > As a data point, calling printf ("%s", p) does lead to a segfault in Glibc > for a null p because GCC turns the call into puts(p) Only when optimization is enabled. I'm not suggesting we should print "(null)", that would be crazy, I'm just demonstrating that expecting a segfault for this kind of error is not a safe or valid assumption. > which doesn't have the > same feature (see https://sourceware.org/bugzilla/show_bug.cgi?id=5618 for > the background). > > I think most users prefer invalid uses of pointers to fail loudly so they > can be caught early. Few users expect output functions to fail, and even > fewer bother to check for failures when writing to standard streams. Agreed, but this code has been present since r53839 in 2002, and there's an explicit test for that behaviour: testsuite/27_io/basic_ostream/inserters_character/char/8.cc This is not an accident, it's a supported feature (and arguably documented, by the code and the test). > Besides the inserter without the test resulting in more efficient code and > GCC emitting a warning when a null pointer is passed to it (it doesn't now > even though it should because of the strlen call), leaving it to the > compiler to deal with can also lead to better code downstream thanks to > -fdelete-null-pointer-checks. I think there would need to be a period of deprecation and an optional assertion enabled by _GLIBCXX_ASSERTIONS before we can do that (assuming we want to). If we were starting from scratch I wouldn't add the check (e.g. see the patch at https://gcc.gnu.org/ml/gcc-patches/2018-06/msg00768.html that I proposed today) but it's far too late to just turn this into a segfault because somebody on stackoverflow found it surprising and doesn't know how to check iostreams for errors.
(In reply to Paul Sanders from comment #12) > Any interest? Good grief, no. That would generate even worse code than what we have now, and it's possible to test for badbit without enabling exceptions. You keep making assumptions about what everyone else's programs do, and we have no way of knowing what the majority of libstdc++ users expect from this code, or how much code relies on the current behaviour.
(In reply to Jonathan Wakely from comment #13) > This is not an accident, it's a supported feature (and arguably documented, > by the code and the test). See also Bug 6518 which changed the segfault to setting failbit (and introduced a regression) and Bug 6750 which introduced the current behaviour.
Interesting, I can see why you don't want to change the behaviour again. It's just a shame it ever did anything other than SEGFAULT in the first place but as you point out, it's been the way it is for a long time now and changing it would be foolhardy. Just to finish off (I see that the decision is made): > this code has been present since r53839 in 2002 ... [and] it's far too late to just turn this into a segfault because somebody on stackoverflow found it surprising and doesn't know how to check iostreams for errors. I'm sure myself and the OP aren't the only ones ever to have been 'surprised' by this, and more will follow. I will update my answer at SO to explain what is really happening and how to fix it (the original code wasn't my own). Thank you for taking the time to lay things out for me, I learnt a lot here, even if I still don't like the way ostream behaves under these circumstances. It seems to me that badbit's job is to reflect whether or not the stream is in a usable state, and not get set just because something strange has been passed to the stream which does it no actual harm. It (badbit) just got abused back in 2002, so to speak, and now we're stuck with it. And finally, I owe you an apology for ever thinking that ostream would silently terminate your program like that; I posted one on SO. I hope it does some good, I don't set out to aggravate you guys.
*** Bug 109891 has been marked as a duplicate of this bug. ***
(In reply to Jonathan Wakely from comment #13) > I think there would need to be a period of deprecation and an optional > assertion enabled by _GLIBCXX_ASSERTIONS before we can do that (assuming we > want to). As I said in the PR 109891 dup, _GLIBCXX_DEBUG_PEDANTIC exists for precisely this kind of case: https://gcc.gnu.org/onlinedocs/libstdc++/manual/debug_mode_semantics.html So let's add that for now.
*** Bug 111729 has been marked as a duplicate of this bug. ***
Silently ruining the behavior of the rest of a program and leaving the programmer to pull their hair out over what on earth is happening seems very un-ideal behavior. This is a very easy mistake to make and the current behavior makes tracking down that mistake exceedingly difficult. I do recognize the concerns here about existing programs and crashing being arguably less friendly, but something should be done here. Libc++ and msvc's stl both segfault and there's some value in making the behavior here consistent. (In reply to Jonathan Wakely from comment #13) > I'm not suggesting we should print "(null)", that would be crazy Maybe, but it's also a nice middle ground between silently corrupting the stream and crashing the program. Throwing an exception, even only in debug, also seems reasonable as existing programs may be able to recover from the exception without crashing.
Another option might be just do nothing and don't set the badbit, just pretend it's an empty string. This shouldn't break existing programs and would at least be something a programmer could more easily track down.