‘hdrblobInit’: check pointer is 8-byte aligned

[DemiMarie]

Otherwise, we will dereference a misaligned pointer, which is undefined
behavior.

[pmatilai]

This is because the void pointer is then cast to an int32_t pointer, right?

Please put that align check into a macro and refer to the actual thing it depends on, that'll make it much more self-explanatory than this random/magic-looking & 7. Ie something to the tune of if (chkAlign(uh, sizeof(blob->ie)). The sizeof() can be changed to alignof() when we some day go to C11 (or better yet, you could use alignof() if available and define a poor mans version on sizeof() if not)

[pmatilai]

Um, seems I wasn't quite awake yesterday. There's no universal law that says that every pointer must be 8-byte aligned. Alignment depends on the architecture, pointer sizes and all. Like I said, refer to the thing that the alignment depends on, ie blob->ie. It's size and alignment is most certainly not equal to uint64_t everywhere.

Also I believe alignof() is defined as a macro in the standard, so you could probably just test for that directly rather than test for version - testing explicit feature is always better than versions.

The other thing I wonder about is that this void pointer has been in rpm in 20+ years with no issues on any architecture. So I think this could use an explanation why exactly this is needing that alignment check now. When referring to things like compiler standards, it's good practise to point the relevant section that declares the undefined behavior - not everybody sleeps with compiler standards under their pillow I certainly do not.
Also, where do you see that unaligned pointer to be coming from? This is typically memory malloc()'ed by ourselves, and if an API user throws some random crap at that function, seems to me that their problem.

[DemiMarie]

Um, seems I wasn't quite awake yesterday. There's no universal law that says that every pointer must be 8-byte aligned. Alignment depends on the architecture, pointer sizes and all. Like I said, refer to the thing that the alignment depends on, ie blob->ie. It's size and alignment is most certainly not equal to uint64_t everywhere.

Agreed. That said, hdrchkAlign assumes that if the offset is 8-byte aligned, then blob->ei + offset will be as well. That is true if (and only if) blob->ei is itself 8-byte aligned.

Also I believe alignof() is defined as a macro in the standard, so you could probably just test for that directly rather than test for version - testing explicit feature is always better than versions.

It is, but that would require changing configure.ac to check for stdalign.h.

The other thing I wonder about is that this void pointer has been in rpm in 20+ years with no issues on any architecture. So I think this could use an explanation why exactly this is needing that alignment check now. When referring to things like compiler standards, it's good practise to point the relevant section that declares the undefined behavior - not everybody sleeps with compiler standards under their pillow I certainly do not.

Probably for the reason you mentioned in your next paragraph: I suspect that headerImport is very rarely called outside of RPM itself.

Also, where do you see that unaligned pointer to be coming from? This is typically memory malloc()'ed by ourselves, and if an API user throws some random crap at that function, seems to me that their problem.

Personally, if I see a C API taking a void pointer and length, I assume that there are no alignment requirements unless the documentation states that there are.

[pmatilai]

So bottom line, this is all theoretical.

While it's okay to improve theoretical cases too, it is not exactly high-priority work. Which is why asked you to make those reproducer cases available so I can prioritize.

[DemiMarie]

So bottom line, this is all theoretical.

While it's okay to improve theoretical cases too, it is not exactly high-priority work. Which is why asked you to make those reproducer cases available so I can prioritize.

I don’t actually have reproducers for most of them. The more severe cases have been reported privately via secalert@redhat.com, as they have security implications.

[pmatilai]

For heavens sake. All along I've asking to make available the reproducer cases that you DO HAVE. Nothing else.

[DemiMarie]

For heavens sake. All along I've asking to make available the reproducer cases that you DO HAVE. Nothing else.

Sorry; this was a misunderstanding on my part. Uploaded in the other thread.

[DemiMarie]

@pmatilai does this look okay? I understand it isn’t high priority, but it makes the API that little bit safer.

[pmatilai]

On a second thought, NAK. There are too many redundant checks in the code as it is, and adding these kind of just-in-case checks doesn't do anything to help spot the actually crucial missing ones. Forest from the trees, you know.

If somebody passes a random non-malloced address to headerImport() or friends, that's a bug in the caller code.

Apologies for asking you to do extra work and then reject anyway, but there's simply too much on the plate as it is.

[DemiMarie]

On a second thought, NAK. There are too many redundant checks in the code as it is, and adding these kind of just-in-case checks doesn't do anything to help spot the actually crucial missing ones. Forest from the trees, you know.

If somebody passes a random non-malloced address to headerImport() or friends, that's a bug in the caller code.

Apologies for asking you to do extra work and then reject anyway, but there's simply too much on the plate as it is.

That’s understandable. I will file a separate PR with a documentation fix, so that callers know what to do.