The issue is that createCXString has an optimization to check if the
original string is nul-terminated. (See CXString.cpp:46) If the
original StringRef is nul-terminated, then it does not duplicate the
string. This is a good optimization in general, but it requires
reading one past end byte of the string. In this case, this byte just
happens to be uninitialized, because there was no need to initialize
it to create a StringRef that was passed to createCXString.
But this access can, in fact, trap, if it happens to be on the next
page that is unallocated.
Another possible case is when that byte happens to be nul during
createCXString, but is modified afterwards. When the libclang user
code reads the string, it will read garbage at the end.
The proper fix, I think, is to remove this optimization. Only this
way we can guarantee correctness. But I brought up this discussion
because doing so can have a big performance impact. (And somehow we
got away with reading one past end for a long time...)
The immediate fix for this failure is easy and it just proves that the
real issue is inside createCXString:
The issue is that createCXString has an optimization to check if the
original string is nul-terminated. (See CXString.cpp:46) If the
original StringRef is nul-terminated, then it does not duplicate the
string. This is a good optimization in general, but it requires
reading one past end byte of the string. In this case, this byte just
happens to be uninitialized, because there was no need to initialize
it to create a StringRef that was passed to createCXString.
But this access can, in fact, trap, if it happens to be on the next
page that is unallocated.
Another possible case is when that byte happens to be nul during
createCXString, but is modified afterwards. When the libclang user
code reads the string, it will read garbage at the end.
The proper fix, I think, is to remove this optimization.
I, for one, agree.
Only this
way we can guarantee correctness. But I brought up this discussion
because doing so can have a big performance impact. (And somehow we
got away with reading one past end for a long time...)
The immediate fix for this failure is easy and it just proves that the
real issue is inside createCXString:
The issue is that createCXString has an optimization to check if the
original string is nul-terminated. (See CXString.cpp:46) If the
original StringRef is nul-terminated, then it does not duplicate the
string. This is a good optimization in general, but it requires
reading one past end byte of the string. In this case, this byte just
happens to be uninitialized, because there was no need to initialize
it to create a StringRef that was passed to createCXString.
But this access can, in fact, trap, if it happens to be on the next
page that is unallocated.
Another possible case is when that byte happens to be nul during
createCXString, but is modified afterwards. When the libclang user
code reads the string, it will read garbage at the end.
Yes, this is bad.
The proper fix, I think, is to remove this optimization. Only this
way we can guarantee correctness. But I brought up this discussion
because doing so can have a big performance impact. (And somehow we
got away with reading one past end for a long time...)
We could change how CXString works; instead of eagerly malloc'ing in case of a StringRef, have it stored in a "StringRef-kind" form and malloc when clang_getCString is called.
But also add a new function like clang_getStringWithLength, or something, for callers that can use that to avoid the extra malloc.
We could change how CXString works; instead of eagerly malloc'ing in case of a StringRef, have it stored in a "StringRef-kind" form and malloc when clang_getCString is called.
This will add an extra backing storage mode to CXString, let's call
that CXS_UnmanagedWithLength. In this mode we will store the pointer
in 'data', and length will be stored in the upper bits of
'private_flags'. But who will own the memory returned by
clang_getCString? We can not change the original CXString from
CXS_UnmanagedWithLength to CXS_Malloc because CXString is a value
type. Or did I misunderstand something?
We could change how CXString works; instead of eagerly malloc'ing in case of a StringRef, have it stored in a "StringRef-kind" form and malloc when clang_getCString is called.
This will add an extra backing storage mode to CXString, let's call
that CXS_UnmanagedWithLength. In this mode we will store the pointer
in 'data', and length will be stored in the upper bits of
'private_flags'. But who will own the memory returned by
clang_getCString? We can not change the original CXString from
CXS_UnmanagedWithLength to CXS_Malloc because CXString is a value
type. Or did I misunderstand something?
You are right, it is so easy to get a copy of it and trigger a memory leak unintentionally.
I'll do some measurements to see if it makes much difference to malloc on every StringRef (in the meantime any suggestions on how to avoid the malloc would be greatly appreciated).
We could change how CXString works; instead of eagerly malloc'ing in case of a StringRef, have it stored in a "StringRef-kind" form and malloc when clang_getCString is called.
This will add an extra backing storage mode to CXString, let's call
that CXS_UnmanagedWithLength. In this mode we will store the pointer
in 'data', and length will be stored in the upper bits of
'private_flags'. But who will own the memory returned by
clang_getCString? We can not change the original CXString from
CXS_UnmanagedWithLength to CXS_Malloc because CXString is a value
type. Or did I misunderstand something?
You are right, it is so easy to get a copy of it and trigger a memory leak unintentionally.
I'll do some measurements to see if it makes much difference to malloc on every StringRef (in the meantime any suggestions on how to avoid the malloc would be greatly appreciated).
We can add an indirection layer. Let's add a mode CXS_Lazy. 'data'
would point to a struct:
Then CXString would remain a value type, and all copies would point to
the same underlying data.
I like it! We can also enhance the mechanism a bit to be able to use a free list of those so that we avoid malloc'ing them for the normal use pattern of having a CXString as a temporary object to get at the underlying string, what do you think ?
A good malloc implementation should already do that. Anyway, we do
have ArrayRecycler, so we can use it here. But I don't see a good
place to put an instance of ArrayRecycler. We can not make it static
because of thread safety.
We could change how CXString works; instead of eagerly malloc'ing in case of a StringRef, have it stored in a "StringRef-kind" form and malloc when clang_getCString is called.
This will add an extra backing storage mode to CXString, let's call
that CXS_UnmanagedWithLength. In this mode we will store the pointer
in 'data', and length will be stored in the upper bits of
'private_flags'. But who will own the memory returned by
clang_getCString? We can not change the original CXString from
CXS_UnmanagedWithLength to CXS_Malloc because CXString is a value
type. Or did I misunderstand something?
You are right, it is so easy to get a copy of it and trigger a memory leak unintentionally.
I'll do some measurements to see if it makes much difference to malloc on every StringRef (in the meantime any suggestions on how to avoid the malloc would be greatly appreciated).
We can add an indirection layer. Let's add a mode CXS_Lazy. 'data'
would point to a struct:
Then CXString would remain a value type, and all copies would point to
the same underlying data.
I like it! We can also enhance the mechanism a bit to be able to use a free list of those so that we avoid malloc'ing them for the normal use pattern of having a CXString as a temporary object to get at the underlying string, what do you think ?
A good malloc implementation should already do that. Anyway, we do
have ArrayRecycler, so we can use it here. But I don't see a good
place to put an instance of ArrayRecycler. We can not make it static
because of thread safety.
How about putting the instance (or whoever owns it) in the indirection struct ?