return "const char *"

[Mark]

Is it a valid and portable to return strings as in the following code?

#include <stdio.h>

static const char *errno2str(int err)
{
switch (err) {
case 1:
return "Error 1 occured";
case 2:
return "Error 2 occured";
default:
return "Unknown error occured";
}
}

int main(void)
{
printf("%s\n", errno2str(1));
printf("%s\n", errno2str(2));
printf("%s\n", errno2str(5));
return 0;
}

The reason I'm asking is that compilation with
"gcc -ansi -pedantic -W -Wall" goes clean and smooth, but splint spits this
out:
Splint 3.1.1 --- 11 Sep 2006

x.c: (in function errno2str)
x.c:7:11: Observer storage returned without qualification: "Error 1 occured"
Observer storage is transferred to a non-observer reference. (Use
-observertrans to inhibit warning)
x.c:7:11: Storage becomes observer
x.c:9:11: Observer storage returned without qualification: "Error 2 occured"
x.c:9:11: Storage becomes observer
x.c:11:11: Observer storage returned without qualification:
"Unknown error occured"
x.c:11:11: Storage becomes observer
x.c: (in function main)
x.c:17:17: New fresh storage (type char *) passed as implicitly temp (not
released): errno2str(1)
A memory leak has been detected. Storage allocated locally is not released
before the last reference to it is lost. (Use -mustfreefresh to inhibit
warning)
x.c:18:17: New fresh storage (type char *) passed as implicitly temp (not
released): errno2str(2)
x.c:19:17: New fresh storage (type char *) passed as implicitly temp (not
released): errno2str(5)

Finished checking --- 6 code warnings

 

[luserXtrog]

Is it a valid and portable to return strings as in the following code?

#include <stdio.h>

static const char *errno2str(int err)
{
    switch (err) {
        case 1:
            return "Error 1 occured";
        case 2:
            return "Error 2 occured";
        default:
            return "Unknown error occured";
    }

}

int main(void)
{
    printf("%s\n", errno2str(1));
    printf("%s\n", errno2str(2));
    printf("%s\n", errno2str(5));
    return 0;

}

The reason I'm asking is that compilation with
"gcc -ansi -pedantic -W -Wall" goes clean and smooth, but splint spits this
out:
Splint 3.1.1 --- 11 Sep 2006

x.c: (in function errno2str)
x.c:7:11: Observer storage returned without qualification: "Error 1 occured"
  Observer storage is transferred to a non-observer reference. (Use
  -observertrans to inhibit warning)
   x.c:7:11: Storage becomes observer
x.c:9:11: Observer storage returned without qualification: "Error 2 occured"
   x.c:9:11: Storage becomes observer
x.c:11:11: Observer storage returned without qualification:
              "Unknown error occured"
   x.c:11:11: Storage becomes observer
x.c: (in function main)
x.c:17:17: New fresh storage (type char *) passed as implicitly temp (not
              released): errno2str(1)
  A memory leak has been detected. Storage allocated locally is not released
  before the last reference to it is lost. (Use -mustfreefresh to inhibit
  warning)
x.c:18:17: New fresh storage (type char *) passed as implicitly temp (not
              released): errno2str(2)
x.c:19:17: New fresh storage (type char *) passed as implicitly temp (not
              released): errno2str(5)

Finished checking --- 6 code warnings

This came up here quite recently.
The quick fix is:
static /*@observer@*/ const char * ...

 

[luserXtrog]

This came up here quite recently.
The quick fix is:
static /*@observer@*/ const char * ...

A somewhat different fix is akin to the old punchline,

Doctor. Don't do that.

#include <stdio.h>

const char *errstr[] = {
"Unknown error occured",
"Error 1 occured",
"Error 2 occured"
};
#define BOUND(no) (no>=(sizeof errstr/sizeof *errstr)?0:no)

int main(void)
{
printf("%s\n", errstr[BOUND(1)]);
printf("%s\n", errstr[BOUND(2)]);
printf("%s\n", errstr[BOUND(5)]);
return 0;

}

 

[Nate Eldredge]

Yep, UB all over the place.

As soon as you return from errno2str each of your strings ceases to exist or
IIRC goes out of scope is how the standard puts it. It may work or demons may
fly out of your nose. splint assumes the latter.

While errno2str is running the strings exist but are auto. Once errno2str
returns as auto they don't exist but you returned a pointer to one of them.
Anything can now be stored at the memory the pointer points to, so anything can
happen.

That is not correct. String literals, wherever they may occur, have
static storage duration, which means they exist throughout the run of
the program. C99 6.4.5 (5). So the OP's code is fine, and in fact, is
effectively equivalent to your code below.

However, if the `static' keywords were removed from your code below, it
would result in just the sort of undefined behavior you mention. This
might be what you have in mind.

Note that the string appearing in a definition like

{
const char s1[] = "Hello world";
}

is not a string literal, though the syntax is the same, but an
initializer.

Oh, get rid of the -W it is depreciated (for gcc) but add a -Wextra


#include <stdio.h>

static const char *errno2str(int err)
{
static const char e1[] = "Error 1 occured";
static const char e2[] = "Error 2 occured";
static const char e3[] = "Unknown error occured";
switch (err) {
case 1:
return &e1[0];
case 2:
return &e2[0];
default:
return &e3[0];
}
}

Most people would just write `return e1;', etc.

 

[Nate Eldredge]

luserXtrog wrote:

There's nothing to fix. The original code is fine.

I suppose the effect of this "fix" is just to tell splint to shut up.

 

[luserXtrog]

There's nothing to fix. The original code is fine.

The code does work as intended; but I'd hardly call it fine.
Do you truly consider that function to demonstrate the best
way to accomplish its goal?

To me, mapping small integers to strings screams out
"char *[]". Even if you ignore issues of efficient execution,
it's two lines of text to add for each new message; and it's
manually counting the natural numbers where that could be done
automatically with no magic numbers at all.

 

[Martin Ambuhl]

Yep, UB all over the place.

As soon as you return from errno2str each of your strings ceases to exist or
IIRC goes out of scope is how the standard puts it. It may work or demons may
fly out of your nose. splint assumes the latter.

Are you purposely lying to the original poster, or are you just ignorant?

 

[jameskuyper]

Nate Eldredge wrote:
....

Note that the string appearing in a definition like

{
const char s1[] = "Hello world";
}

is not a string literal, though the syntax is the same, but an
initializer.

That's not how the standard describes it:

" An array of character type may be initialized by a character string
literal, optionally
enclosed in braces. Successive characters of the character string
literal (including the
terminating null character if there is room or if the array is of
unknown size) initialize the
elements of the array." (6.7.8p14)

 

[luserXtrog]

That's the second post from someone in the past month or so reporting an
apparent problem that turns out due to splint. Seems the better fix is to
just remove splint from one's testing tools, rather than clutter one's
code with crap like that.

That's a rather extreme reaction. A few false hits is fair price
to pay for semantic checking. It finds bad stuff too. And this is,
in fact, the exact same error as the previous post! That's how I
already knew the answer.

 

[Nate Eldredge]

Nate Eldredge wrote:
...

Note that the string appearing in a definition like

{
const char s1[] = "Hello world";
}

is not a string literal, though the syntax is the same, but an
initializer.

That's not how the standard describes it:

" An array of character type may be initialized by a character string
literal, optionally
enclosed in braces. Successive characters of the character string
literal (including the
terminating null character if there is room or if the array is of
unknown size) initialize the
elements of the array." (6.7.8p14)

Oops.

What I meant is that that behavior discussed in 6.4.5 (5) ("The
multibyte character sequence is then used to initialize an array of
static storage duration and length just sufficient to contain the
sequence") does not apply in that context.

Actually, I suppose maybe it does. E.g. in

void foo(void) {
char msg[] = "Hello, world!";
}

the multibyte character sequence "Hello, world!" is used to initialize
an array of static storage duration. That array in turn is used to
initialize `msg'. This would actually be a reasonable way to implement
it: create an anonymous static array and copy it into `msg' at runtime.
However, since there is no other way to access the anonymous static
array, an implementation would be free, under the as-if rule, to
dispense with it and initialize `msg' in some other way (e.g. a series
of store-immediate instructions).

My point was that 6.4.5 should not be misread as saying that an array
like `msg' which is initialized by a string literal is therefore
automatically static (no pun intended). The "array of static storage
duration" of 6.4.5 and the "array of character type" of 6.7.8 are two
different arrays, in principle.

 

[Nate Eldredge]

That's the second post from someone in the past month or so reporting an
apparent problem that turns out due to splint. Seems the better fix is to
just remove splint from one's testing tools, rather than clutter one's
code with crap like that.

Well, static analysis tools can never be perfect (thanks, Dr. Turing!),
so it's not surprising that they work better when you help them along.
"Tell splint to shut up" was an oversimplification; I assume the comment
conveys some information about the behavior or use of the function which
helps split to deduce that the code is correct. Not knowing much about
splint, nor what it might mean by "observer," I can't comment further.

 

[Phil Carmody]

There's nothing to fix. The original code is fine.

There is something to fix - it's called "splint".

I'm pretty sure Coverity's on LSD sometimes too...

Phil

 

[James Kuyper]

luserXtrog wrote: ....

Yes.
The purpose of the function
was to generate questionable warnings with splint.

The point is, those warning are intended to point out code that, in the
opinion of the designers of splint, is unlikely to be the best way to
accomplish a more reasonable goal. In order to demonstrate not only that
these warnings can be triggered, but that they are also questionable,
you should present code that is clearly the best way to achieve some
other goal, and also incidentally triggers these warnings. If you find
yourself unable to come up with such code, then pointing that out is
precisely what these warnings are for.

Note: I'm judging neither the reasonableness of your code, nor the
reasonableness of the warnings - both issues require more careful
examination than I've got time for. I'm merely commenting upon the
connection between the two.

 

[jameskuyper]

James Kuyper wrote: ....

No.

Splint reported "A memory leak has been detected"
for code which had no memory leak.

Splint was wrong; that's the point.

The memory leak detection was bogus, but it was based upon making some
assumptions about the best way to write such a program, assumptions
that were violated by this code. The first message identified the
failure to obey those assumptions; the memory leak "detection" was a
consequence of splint being confused by that violation. This is the
same kind of problem you get into when a compiler gives you a valid
warning about one defect, and then misinterprets the rest of the
source code because that defect left it confused.

I don't necessarily agree with the idea that this is a bad way to
write code (I'm still working on figuring out what they mean by
"observer"); but that is the idea that the designers of splint were
using.