Opened 5 years ago

Closed 4 years ago

#1096 closed Bug Report (fixed)

Alignment issue with SerdReader stack

Reported by: Matt Fischer Owned by: David Robillard
Priority: major Component: Serd
Keywords: Cc:

Description

The stack used to implement SerdReader? does not make any alignment guarantees, which can cause problems on architectures like ARM which do not always allow for unaligned memory access.

I've attached a patch which I think should fix the issue. It works by adding a new function, called serd_stack_align(), which can be used to pad out the stack to a given alignment boundary, thus guaranteeing that the next data to be pushed onto it will be aligned correctly. From what I've been able to determine, this padding doesn't cause any problems because it will all get discarded when we pop down to the next node below it, but please let me know if I missed something there.

Attachments (2)

0001-Fix-stack-alignment.patch (1.4 KB) - added by Matt Fischer 5 years ago.
serd_stack_alignment.diff (3.3 KB) - added by David Robillard 5 years ago.

Download all attachments as: .zip

Change History (9)

Changed 5 years ago by Matt Fischer

comment:1 Changed 5 years ago by David Robillard

I see the issue that SerdNode?'s in the stack aren't aligned, but changing the stack behaviour worries me a bit. It was possible to e.g.

push_node() push_byte('h') push_node() Whatever pop_node() push_byte('i')

and have a node on the top of the stack with text "hi". The added alignment weakens the assertion in read_object() because this is no longer the case. In other words, the invariant "the top of the stack is the end of the top node on the stack" is violated by this.

That said, I don't think the reader currently ever does that, and I can't think of a way of popping to the correct size, since the previous top's size is unknown...

comment:2 Changed 5 years ago by Matt Fischer

Right, that's basically what I was worried about. It's kind of a strange case, but if you did need to support that, then this strategy wouldn't work. The only other idea that came to mind was for each node to store a pointer to where the stack was when it was created, or something like that. That would allow you to keep track of any padding you added, but it seemed like overkill to add a whole new structure member to track something that you mostly already knew.

comment:3 Changed 5 years ago by Matt Fischer

Ping. Any other thoughts about this?

comment:4 Changed 5 years ago by David Robillard

Apologies. I needed to make a release and this change makes me a bit nervous, so I pushed one out before committing it.

I feel like a better stack design could avoid this problem entirely, but maybe that's a bit too ambitious. I'll look into it and decide what to do before the next release.

P.S. Is there an easy way I can test this without having hardware with that limitation? Perhaps an emulator?

comment:5 Changed 5 years ago by Matt Fischer

The key failure is in push_node_padded() when casting mem into node, since that's the line that technically breaks the C spec by creating a pointer to an aligned type which may not actually be aligned. So probably the easiest way to test is just to check mem at that point and see if it's not aligned. That will catch exactly those cases which on ARM would eventually lead to an alignment trap when the pointer gets used.

Changed 5 years ago by David Robillard

Attachment: serd_stack_alignment.diff added

comment:6 Changed 5 years ago by David Robillard

How about this? It fixes the alignment issue but still pops to the correct previous top, at the cost of 1 byte on the stack.

There's still some strict aliasing violations (I see no way of completely avoiding them while still having a single stack), but I think it should be aligned. No ARM around to test on, though.

comment:7 Changed 4 years ago by David Robillard

Resolution: fixed
Status: newclosed

Turns out that this is undefined behaviour which is caught by UBSan. I've applied the above fix as e2d1f43/serd and the test suite now runs clean, so it should be fixed on ARM.

Your fix has less overhead and seems fine, but I feel better about having a stack that pops correctly even if the reader doesn't currently make use of that ability.

Thanks.

Note: See TracTickets for help on using tickets.