Live data from Hacker News

ELF hash function may overflow

maskray.me

1–10 of 41 posts

Re: ELF hash function may overflow

#2
If someone checked in that code, it would definitely fail my code review. I understand back in the day it was different, but today there should be a lot of named intermediates. Additionally, `long` and any such keywords should not make it into any commit unless the commit explains 1) why its needed and 2) how, with any standard conforming implementation, it couldnt possibly cause a bug.

As always in C programming, the bugs arise from people doing stuff that any sane guideline tells them to not do.

Re: ELF hash function may overflow

#3
post #2

If someone checked in that code, it would definitely fail my code review. I understand back in the day it was different, but today there should be a lot of named intermediates. Additionally, `long` and any such keywords should not make it into any commit unless the commit explains 1) why its needed and 2) how, with any standard conforming implementation, it couldnt possibly cause a bug. As always in C programming, th…

For all its advantages, C is unfortunately so ripe with stuff that any sane guideline would recommend not to do that it can hard to follow through.

Though I agree in this case this would never have passed a modern review.

Re: ELF hash function may overflow

#4
post #2

If someone checked in that code, it would definitely fail my code review. I understand back in the day it was different, but today there should be a lot of named intermediates. Additionally, `long` and any such keywords should not make it into any commit unless the commit explains 1) why its needed and 2) how, with any standard conforming implementation, it couldnt possibly cause a bug. As always in C programming, th…

Named intermediates for what? This function munges a single hash value, h, and then folds its bits 24 to 31, g, into bits 4 to 7. It implements a mathematical formula, basically, and given it’s a hash, there isn’t much meaning to the contents of any pair of parens in that formula. Perhaps *name++, but any C programmer just thinks “next character of name” when they see that, don’t they?

(I’ve seen people code with a lot of intermediates with two- and three-word names, and I just don’t see why, in general. But here especially—hashes are not exactly oases of meaning.)

The 'unsigned long' point is correct in a cross-platform context, of course. If the code is from the SunOS linker, then its authors defined the ABI, so could guarantee 'long' was 32 bits. It’s the Glibc port that was careless.

(Although if I heard the phrase “any standard-conforming implementation” in this context, I’d be tempted to point out that an implementation doesn’t have to provide uint32_t at all, though a POSIX 2008 one does, and may have 33-bit ints, automatically promoting the uint32_t to signed int and immediately hitting UB on overflow. Either use C23 _BitInt(32), not subject to promotions for precisely this reason, or add +0U as needed to force the hypothetical signed int to unsigned.)

Re: ELF hash function may overflow

#5
post #2

If someone checked in that code, it would definitely fail my code review. I understand back in the day it was different, but today there should be a lot of named intermediates. Additionally, `long` and any such keywords should not make it into any commit unless the commit explains 1) why its needed and 2) how, with any standard conforming implementation, it couldnt possibly cause a bug. As always in C programming, th…

Another perspective is that easily preventable bugs in C arise because the compiler doesn’t stop people from doing them.

Re: ELF hash function may overflow

#6
post #2

If someone checked in that code, it would definitely fail my code review. I understand back in the day it was different, but today there should be a lot of named intermediates. Additionally, `long` and any such keywords should not make it into any commit unless the commit explains 1) why its needed and 2) how, with any standard conforming implementation, it couldnt possibly cause a bug. As always in C programming, th…

For all its advantages, C is unfortunately so ripe with stuff that any sane guideline would recommend not to do that it can hard to follow through. Though I agree in this case this would never have passed a modern review.

One only needs to compare C programming manuals with the programming manuals from systems programming languages being developed outside Bell Labs.

Also note that C author's were naturally aware of these issues and created lint in 1979.

Now getting people to use such tooling is another matter, apparently 50 years weren't enough.

Re: ELF hash function may overflow

#7
post #2

If someone checked in that code, it would definitely fail my code review. I understand back in the day it was different, but today there should be a lot of named intermediates. Additionally, `long` and any such keywords should not make it into any commit unless the commit explains 1) why its needed and 2) how, with any standard conforming implementation, it couldnt possibly cause a bug. As always in C programming, th…

The sized uint sure, I get that.

But the rest of the code? I guess you could make it so the dereference and pointer-increment happen separately. And for someone unfamiliar you could expand out the loop condition.

But ultimately it's a hash function. How would you write it?

Re: ELF hash function may overflow

#8
I chased that rabbit hole briefly and it's not very clear that the hashed value is required to be > It seems obvious that on 32-bit and 64-bit systems, the function should not give different results

and a commit to mask off the low bits in an implementation elsewhere.

Well, maybe that would be convenient, but overall it seems unimportant. It's necessary for the tool writing the table and the tool reading it to agree but cross compilation is absolutely full of hazards like this anyway.

The code looks fine to me for what that's worth. I can see the assignment in the if being contentious.

Re: ELF hash function may overflow

#9
post #5
post #2

If someone checked in that code, it would definitely fail my code review. I understand back in the day it was different, but today there should be a lot of named intermediates. Additionally, `long` and any such keywords should not make it into any commit unless the commit explains 1) why its needed and 2) how, with any standard conforming implementation, it couldnt possibly cause a bug. As always in C programming, th…

Another perspective is that easily preventable bugs in C arise because the compiler doesn’t stop people from doing them.

I don't understand what it is you expect the compiler to stop. There's nothing actually wrong with the code. It's just written in a platform-dependent manner.

Re: ELF hash function may overflow

#10
ELF is way too complex and not really adapted anymore.

We should start to deprecate DT_NEEDED and make dlopen/dlsym/dlclose (maybe, dlvsym) hard symbols in the loader.

And game devs should stop using main() as some genius glibc dev did add a new libc_start_main version in 2.34. Namely, any game executable linked with a glibc from 2.34 will refuse to load on system with a previous glibc.

Actually, game binaries should be pure ELF64 binaries (not using main()) which "libdl" (dlopen/dlsym/dlclose) everything they need from the system. And of course, as much as possible should be statically linked (I think this is what unity is doing, but unreal/godot have a big issue: the static libstdc++ which, as of late, does not libdl anything from the system).

Post reply on HN