Closed matoro closed 8 months ago
There's a TODO in the code for this :)
I'm not sure how we should get the proper alignment requirements; but here's an attempt making a slightly less dump assumption (that is, that alignment for void*
is OK for everything, rather than sizeof(int)
), is this enough?
diff --git a/main/nestlevel.c b/main/nestlevel.c
index 3ce87df09..2387a6d8c 100644
--- a/main/nestlevel.c
+++ b/main/nestlevel.c
@@ -20,8 +20,22 @@
#include <string.h>
-/* TODO: Alignment */
-#define NL_SIZE(nls) (sizeof(NestingLevel) + (nls)->userDataSize)
+/* This is a dummy struct for guessing the alignment for the user data */
+typedef struct
+{
+ struct NestingLevel nl; /* not a pointer */
+ /* We make the assumption that void* alignment requirements are OK for
+ * everything else, which is not guaranteed (is it?), but usually are.
+ * TODO: review this alignment. GObject uses (2*sizeof(size_t)) alignment,
+ * saying it borrows it from glibc, but the implementation is a bit more
+ * generic than this, so I'm not sure it's actually better */
+ void *ptr;
+} NestingLevelAlignmentHelper;
+
+
+/* account for the user data alignment if we have user data, otherwise allocate
+ * exactly what's needed not to waste memory for unneeded alignment */
+#define NL_SIZE(nls) ((nls)->userDataSize ? (offsetof(NestingLevelAlignmentHelper, ptr) + (nls)->userDataSize) : sizeof(NestingLevel))
#define NL_NTH(nls,n) (NestingLevel *)(((char *)((nls)->levels)) + ((n) * NL_SIZE (nls)))
/*
@@ -73,7 +87,7 @@ extern NestingLevel * nestingLevelsPush(NestingLevels *nls, int corkIndex)
nl->corkIndex = corkIndex;
if (nls->userDataSize > 0)
- memset (nl->userData, 0, nls->userDataSize);
+ memset (nestingLevelGetUserData (nl), 0, nls->userDataSize);
return nl;
}
@@ -117,5 +131,7 @@ extern NestingLevel *nestingLevelsGetNthParent (const NestingLevels *nls, int n)
extern void *nestingLevelGetUserData (const NestingLevel *nl)
{
- return (void *)nl->userData;
+ /* user data (if any) is allocated after the nest level itself, possibly with
+ * padding for alignment. */
+ return (void *)(((char *) nl) + offsetof(NestingLevelAlignmentHelper, ptr));
}
diff --git a/main/nestlevel.h b/main/nestlevel.h
index c70c5e7e4..9e77b5185 100644
--- a/main/nestlevel.h
+++ b/main/nestlevel.h
@@ -26,7 +26,8 @@ typedef struct NestingLevels NestingLevels;
struct NestingLevel
{
int corkIndex;
- char userData [];
+ /* user data is allocated at the end of this struct (possibly with some
+ * offset for alignment), get it with nestingLevelGetUserData() */
};
struct NestingLevels
A simple more correct implementation would be to actually have a pointer and allocate memory separately for it, so we just have to assume malloc()
can allocate anything; but it's a tad bad for fragmentation. Yet, it's better than UB I guess…
[edit] Although, I'm just realizing that this might cause issues as well if the size of the user data is not a multiple of a valid alignment for int
itself, in which case we'd have misaligned access to the second nestlevel onward… more alignment required.
Reproduced with:
# yum install libubsan
$ make clean
$ make CFLAGS='-g -O0 -fsanitize=undefined -fno-sanitize-recover=all' LDFLAGS='-fsanitize=undefined -fno-sanitize-recover=all'
$ ./ctags --verbose --options=NONE --fields=-T --optlib-dir=+./Units/parser-ruby.r/simple.rb.d/optlib -o - --options=./Units/parser-ruby.r/simple.rb.d/args.ctags ./Units/parser-ruby.r/simple.rb.d/input.rb
@masatake thanks for the easy reproduction scenario. So, I do reproduce without my patch, and my patch makes it happy. x86_64 here, though.
@b4n Could you make a pull request for the patch? I will merge it. So @matoro can try the change quickly.
Thank you, great hackers.
@masatake will do, I'm just checking whether this is reasonable enough or if I need something a bit better (as mentioned in my edit, this could possibly cause issue if the result overall size doesn't align to something OK again)
Thanks, that gets everything passing here! I can test arbitrary patches, no need for a PR. Let me know if you want me to test a v2.
See #3883. Indeed I had to improve alignment logic, because otherwise it one passed a weird user data size value (e.g. 25), it'd totally mess up the alignment of the second nesting levels onward (until the alignment magically added back to something OK). This wastes more memory for padding, but it's reasonably easy to implement, and I'm guessing that it's OK for this API to be a little less memory-optimized. See the commit message of #3883 for more info, and possible shrinkage if we really want to.
Hi, it would seem that there is an unaligned access issue in the Ruby parser. This is undefined behavior on all platforms, and is completely disallowed (crashes with SIGBUS) on sparc. I used this fact to obtain the backtrace below.
This affects all of the Ruby tests so I just used the simplest one to reproduce it:
GCC UBSAN at least does catch this (output truncated for brevity):
Since this crashes on sparc, I used this to catch the backtrace on exactly where it dies:
I wanted to try and track this down to bisect it, but after checking commits all the way back to 2021 I did not find any point at which the issue did not occur.
Downstream bug: https://bugs.gentoo.org/920066
Let me know if there is any additional info I can provide to help.