Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Improvements to static analysis in GCC 14

developers.redhat.com

Post reply on HN
Print view View original

31–40 of 147 posts

Re: Improvements to static analysis in GCC 14

#31

Post by lelanthran »

laweijfmvo wrote: post #4
sirwhinesalot wrote: post #3

Earlier quoted context omitted.

There's nothing wrong with simple usages of goto. The strxcpy family on the other hand is complete garbage and should never be used for any reason. I'm horrified that they're used in the kernel at all. All of those functions (and every failed attempt at "fixing" them) should have been nuked from orbit.

What's wrong with `strncpy`?

It's not possible to use it safely unless you know that the source string fits in the destination buffer. Every strncpy must be followed by `dst[sizeof dst - 1] = 0`, and even if you do that you still have no idea if you truncated the source string, so you have to put in a further check.

    strncpy (dst, src, (sizeof dst) - 1);
    dst[(sizeof dst) - 1] = 0;
    int truncated = strlen (dst) - strlen (src);
Without the extra two lines after every strncpy, you're probably going to have a a hard to discover transient bug.

Re: Improvements to static analysis in GCC 14

#32

Post by randomdata »

bluGill wrote: post #27
randomdata wrote: post #12

Earlier quoted context omitted.

> Last I checked they're almost as big no-nos as using goto. Huh? Why is goto a no-no? It is there for good reason. I think we all agree with Dijkstra that, in his words, unbridled gotos are harmful, but C's goto is most definitely bridled. I doubt any language created in the last 50+ years has unbridled gotos. That's an ancient programming technique that went out of fashion long ago (in large part because of Dijkstr…

Languages other than C give you options for flow control so that you don't need goto for that. It is a spectrum, if you only use goto to jump to the end of a small function on error it is okay, though I prefer something better in my language. I've seen 30,000 line functions with gotos used for flow control (loops and if branches) - something you can do in C if you are really that stupid and I think we will all agree…

We all agree that you shouldn't write bad code. Not using goto, not using any language construct.

But when unbridled gotos were the only tool in the toolbox, bad code was an inevitability in a codebase of any meaningful size. Not even the best programmer was immune. This is what the "Go to statement considered harmful" paper was about.

It was written in 1968. We listened. We created languages that addressed the concerns raised and moved forward. It is no longer relevant. Why does it keep getting repeated in a misappropriated way?

Re: Improvements to static analysis in GCC 14

#33

Post by tom_ »

1udfx9cf8azi0 wrote: post #23
pwagland wrote: post #21

Earlier quoted context omitted.

Right, but the size of the buffer is given, it doesn't make sense to stomp over end of the callers buffer either, so you can't use pass in something longer than `nbytes` either.

That's what the original check is for: if (nbytes If the buffer isn't large enough to hold *hwrpb, then it already fails. The original check was good, only needed to change the amount of bytes copied to sizeof(*hwrpb).

The original less-than check was deemed incorrect, and was replaced entirely. For good or for ill, it seems the author deems it valid to pass in a value smaller than sizeof *hwrpb, and that many bytes will be dutifully copied. This might form part of some barebones API versioning mechanism.

Re: Improvements to static analysis in GCC 14

#34

Post by randomdata »

lelanthran wrote: post #30
bluGill wrote: post #27

Earlier quoted context omitted.

Languages other than C give you options for flow control so that you don't need goto for that. It is a spectrum, if you only use goto to jump to the end of a small function on error it is okay, though I prefer something better in my language. I've seen 30,000 line functions with gotos used for flow control (loops and if branches) - something you can do in C if you are really that stupid and I think we will all agree…

> Languages other than C give you options for flow control so that you don't need goto for that. The idiom `if (error) goto cleanup` is about the only thing I see goto used for. What flow control replaces that other than exceptions?

> What flow control replaces that other than exceptions?

defer has gained in popularity for that situation.

Re: Improvements to static analysis in GCC 14

#35

Post by sltkr »

1udfx9cf8azi0 wrote: post #23
pwagland wrote: post #21

Earlier quoted context omitted.

Right, but the size of the buffer is given, it doesn't make sense to stomp over end of the callers buffer either, so you can't use pass in something longer than `nbytes` either.

That's what the original check is for: if (nbytes If the buffer isn't large enough to hold *hwrpb, then it already fails. The original check was good, only needed to change the amount of bytes copied to sizeof(*hwrpb).

No, because if nbytes > sizeof(*hwrpb), your version causes the kernel to only write part of the buffer, and then when the app accesses fields at the end of the struct, it would read uninitialized data, which is very bad.

Recall that the API is intended to be used like this:

    struct hwrbp buf;
    getsysinfo(GSI_GET_HWRPB, &buf, sizeof(buf), /* .. */);
At first glance, it might seem unnecessary to pass the buffer size at all, because in theory the user and kernel should agree on what the sizeof(struct hwrbp) is. But the reason it is passed is because there are various reasons why the separately compiled kernel and user binaries might disagree (e.g., incorrect compiler flags, wrong header file being used, struct has changed between different versions, etc.), and it's useful to detect that. So you can make an argument that the most conservative check is:

    if (nbytes != sizeof(\*hwrpb)) return -1;
After all, if the user and kernel disagree on the correct size of the struct, then something is wrong! But allowing nbytes I would agree with you if the kernel had some other mechanism to pass the size of the buffer that was actually filled to the client (like e.g. the read() syscall does) but the getsysinfo() API doesn't return that data, so the kernel must either fill the buffer entirely or return failure.*

Re: Improvements to static analysis in GCC 14

#36

Post by 1udfx9cf8azi0 »

tom_ wrote: post #33
1udfx9cf8azi0 wrote: post #23

Earlier quoted context omitted.

That's what the original check is for: if (nbytes If the buffer isn't large enough to hold *hwrpb, then it already fails. The original check was good, only needed to change the amount of bytes copied to sizeof(*hwrpb).

The original less-than check was deemed incorrect, and was replaced entirely. For good or for ill, it seems the author deems it valid to pass in a value smaller than sizeof *hwrpb, and that many bytes will be dutifully copied. This might form part of some barebones API versioning mechanism.

> The original less-than check was deemed incorrect

It was only deemed incorrect because of an information leak. Not because it's a valid use-case for user space to copy smaller portions of *hwrpb into user space. https://github.com/torvalds/linux/commit/21c5977a836e399fc71...

Re: Improvements to static analysis in GCC 14

#37

Post by Borg3 »

i80and wrote: post #6
laweijfmvo wrote: post #4

Earlier quoted context omitted.

What's wrong with `strncpy`?

strncpy won't always write a trailing nul byte, causing out of bounds reads elsewhere. It's a nasty little fellow. See the warning at https://linux.die.net/man/3/strncpy strlcpy() is better and what most people think strncpy() is, but still results in truncated strings if not used carefully which can also lead to big problems.

#define strncpyz(d,s,l) *(strncpy(d,s,l)+(l))=0

Of course this one is unsafe for macro expansion. But well, its C :)

Re: Improvements to static analysis in GCC 14

#38

Post by sirwhinesalot »

lelanthran wrote: post #30
bluGill wrote: post #27

Earlier quoted context omitted.

Languages other than C give you options for flow control so that you don't need goto for that. It is a spectrum, if you only use goto to jump to the end of a small function on error it is okay, though I prefer something better in my language. I've seen 30,000 line functions with gotos used for flow control (loops and if branches) - something you can do in C if you are really that stupid and I think we will all agree…

> Languages other than C give you options for flow control so that you don't need goto for that. The idiom `if (error) goto cleanup` is about the only thing I see goto used for. What flow control replaces that other than exceptions?

Jumping out of nested loops. Implementing higher level constructs like yield or defer. State machines. Compiler output that uses C as a "cross-platform" assembly language.

All of them are better served with more specialized language constructs but as a widely applicable hammer goto is pretty nice.

I don't expect C to have good error handling or generators any time soon but with goto I can deal with it.

Re: Improvements to static analysis in GCC 14

#40

Post by 1udfx9cf8azi0 »

sltkr wrote: post #35
1udfx9cf8azi0 wrote: post #23

Earlier quoted context omitted.

That's what the original check is for: if (nbytes If the buffer isn't large enough to hold *hwrpb, then it already fails. The original check was good, only needed to change the amount of bytes copied to sizeof(*hwrpb).

No, because if nbytes > sizeof(*hwrpb), your version causes the kernel to only write part of the buffer, and then when the app accesses fields at the end of the struct, it would read uninitialized data, which is very bad. Recall that the API is intended to be used like this: struct hwrbp buf; getsysinfo(GSI_GET_HWRPB, &buf, sizeof(buf), /* .. */); At first glance, it might seem unnecessary to pass the buffer size at…

> No, because if nbytes > sizeof(*hwrpb), your version causes the kernel to only write part of the buffer, and then when the app accesses fields at the end of the struct, it would read uninitialized data, which is very bad.

> I would agree with you if the kernel had some other mechanism to pass the size of the buffer that was actually filled to the client (like e.g. the read() syscall does) but the getsysinfo() API doesn't return that data, so the kernel must either fill the buffer entirely or return failure.

As you mention, this struct is versioned. Userspace can tell how much of the struct was filled by checking the size field (hwrpb->size).

> But allowing nbytes That's a related but separate issue. Backward compatibility can be handled by switching on nbytes or by copying fewer bytes with a carefully designed struct. It's not clear that backward compatibility was the original intention of this code, the original intention more seems to be sanitizing tainted input. This struct has not changed in at least 16 years.

Post reply on HN