Re: Crash with gnome#627420-1.ods in string related function
On Thu, 2013-01-03 at 19:44 +0200, Noel Grandin wrote: We could throw a StringAllocationFailed exception, and catch it near the bottom if the import process, and use that to indicate that the document is corrupt. Sigh - this brings us back to the old chestnut of the impact exceptions have on generated code size, and (lack of) optimisation. I had hoped that when we last discussed it we ended up with a try not to use exceptions at bazillions of call-sites approach. In this case I imagine sanity-checking the string length vs. some sane / known likely bounds might work; one upper-bound for the length of a string is presumably the size of the un-compressed .zip file. Then again your error sounds like pushing against the over-commit goodness; for which there is no real cure :-) ATB, Michael. -- michael.me...@suse.com , Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Crash with gnome#627420-1.ods in string related function
Hello Michael,
Reading this thread, I took a look at sal/rtl/source/strtmpl.cxx
969 static IMPL_RTL_STRINGDATA* IMPL_RTL_STRINGNAME( ImplAlloc )(
sal_Int32 nLen )
970 {
971 IMPL_RTL_STRINGDATA * pData
972 = (sal::static_int_cast sal_uInt32 (nLen)
973= ((SAL_MAX_UINT32 - sizeof (IMPL_RTL_STRINGDATA))
974/ sizeof (IMPL_RTL_STRCODE)))
975 ? (IMPL_RTL_STRINGDATA *) rtl_allocateMemory(
976 sizeof (IMPL_RTL_STRINGDATA) + nLen * sizeof
(IMPL_RTL_STRCODE))
977 : NULL;
978 if (pData != NULL) {
979 pData-refCount = 1;
980 pData-length = nLen;
981 pData-buffer[nLen] = 0;
982 }
983 return pData;
984 }
Since we cast nLen parameter in sal_uInt32, could it help to add an
assert about nLen should be = 0 ?
Of course, we don't expect a length to be negative but perhaps this function
is called with a negative value in very specific cases.
(advantage of the assert, it costs nothing in non debug)
Julien
--
View this message in context:
http://nabble.documentfoundation.org/Re-Crash-with-gnome-627420-1-ods-in-string-related-function-tp4027116p4027774.html
Sent from the Dev mailing list archive at Nabble.com.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Crash with gnome#627420-1.ods in string related function
On Tue, 2013-01-08 at 05:19 -0800, julien2412 wrote: Hello Michael, Reading this thread, I took a look at sal/rtl/source/strtmpl.cxx 969 static IMPL_RTL_STRINGDATA* IMPL_RTL_STRINGNAME( ImplAlloc )( sal_Int32 nLen ) ... Since we cast nLen parameter in sal_uInt32, could it help to add an assert about nLen should be = 0 ? Sounds great to me :-) lots of code paths to there check for a positive length I guess some mass code clean to use sal_uInt32 instead of sal_Int32 for string lengths might be in order at some stage - though perhaps there is some reason that's not obvious to me for strings to have a signed length and to check for it to be 0 around the place: perhaps for some insertion/offset API semantic corner-case reason ? Thanks ! Michael. -- michael.me...@suse.com , Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Crash with gnome#627420-1.ods in string related function
On 01/08/2013 02:19 PM, julien2412 wrote:
Reading this thread, I took a look at sal/rtl/source/strtmpl.cxx
969 static IMPL_RTL_STRINGDATA* IMPL_RTL_STRINGNAME( ImplAlloc )(
sal_Int32 nLen )
970 {
971 IMPL_RTL_STRINGDATA * pData
972 = (sal::static_int_cast sal_uInt32 (nLen)
973= ((SAL_MAX_UINT32 - sizeof (IMPL_RTL_STRINGDATA))
974/ sizeof (IMPL_RTL_STRCODE)))
975 ? (IMPL_RTL_STRINGDATA *) rtl_allocateMemory(
976 sizeof (IMPL_RTL_STRINGDATA) + nLen * sizeof
(IMPL_RTL_STRCODE))
977 : NULL;
978 if (pData != NULL) {
979 pData-refCount = 1;
980 pData-length = nLen;
981 pData-buffer[nLen] = 0;
982 }
983 return pData;
984 }
Since we cast nLen parameter in sal_uInt32, could it help to add an
assert about nLen should be = 0 ?
Such an assert would surely not hurt, but I suspect that there are call
sites that do not catch overflow of computed length values (where such
overflow can lead to wrong values that are negative as well as
non-negative), so such an assert alone would not help catch all the
problematic call sites.
Stephan
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Crash with gnome#627420-1.ods in string related function
On Tuesday 08 of January 2013, Michael Meeks wrote: On Tue, 2013-01-08 at 05:19 -0800, julien2412 wrote: Hello Michael, Reading this thread, I took a look at sal/rtl/source/strtmpl.cxx 969 static IMPL_RTL_STRINGDATA* IMPL_RTL_STRINGNAME( ImplAlloc )( sal_Int32 nLen ) ... Since we cast nLen parameter in sal_uInt32, could it help to add an assert about nLen should be = 0 ? Could help, hopefully shouldn't hurt. Sounds great to me :-) lots of code paths to there check for a positive length I guess some mass code clean to use sal_uInt32 instead of sal_Int32 for string lengths might be in order at some stage - though perhaps there is some reason that's not obvious to me for strings to have a signed length and to check for it to be 0 around the place: perhaps for some insertion/offset API semantic corner-case reason ? It is a fallacy that using an unsigned variable to represent values that cannot be negative actually buys you safety. E.g. make a function that takes unsigned int, pass -1 to it and see how the silly C/C++ int-unsigned int promotion lets you get away with it. Even without a warning with GCC, apparently -Wconversion doesn't catch this in 4.7, Clang warns with -Wsign-conversion, but that triggers sooo many warnings, because signed types are simply inevitable. There is one thing that you will get though, and that's comparison warnings about mixing signed and unsigned types, and all the useless casts in our code that hide them. It's not like OUString needs to hold a string that's more than 2G characters long anyway. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Crash with gnome#627420-1.ods in string related function
On Thursday 03 of January 2013, Markus Mohrhard wrote: 2013/1/3 Lubos Lunak l.lu...@suse.cz: On Thursday 03 of January 2013, Markus Mohrhard wrote: Hey, while going through the list of calc documents crashing during import I came across gnome#627420-1.ods which creates an insanely large OUStringBuffer that ultimately leads to a crash. Since I believe we have quite a few places contain such problems. I wanted to ask if we should not try to find a solution in the string classes instead of having crashs with such documents from time to time. The question is, what kind of solution do you expect? Presumably the crash was because the allocation failed and the assert was a no-op because of non-debug build, leading to NULL pointer dereference. So probably the only thing we can do is have the assert always active, changing the crash to a different kind of crash, but that seems to be about it. Just to cear somethings up. I do have a dbgutil/debug build and it did not return a null pointer. It returned a pointer to some invalid memory which later results in the crash. Memory allocation functions are not supposed to return invalid memory just like that. Either the crash was because of memory overcommit (which is unlikely to cause a crash right after the allocation, and these days Linux distros do not enable it by default anyway), or the allocation function is buggy (which seems unlikely as well, ImplAlloc even checks for overflows). So I think it would help to know what actually went wrong. Please also note that the document is perfectly fine and valid according to ODF and my fix for it is for example now only a random limit for the text length. I have no idea how to find a general solution but I know that it would be good to have a way to prevent crashes with these documents. It sounds much better to find a way to prevent the crash in these documents on the string implementation than hope to find all places in our import code. While it might be a good idea to fix the import and check the input we will always have new import code where we forget to add these additional dafety checks. A string that is several ten million characters long is a good indicator for a potential problem. The point is, I don't think you can achieve that, which is why I asked what kind of solution you expect. If you alter the function to return a smaller/empty buffer in case of such a big size request, in some cases this call will be followed by code which will expect the buffer to be as big as requested and access it anyway (e.g. OUStringBuffer). -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Crash with gnome#627420-1.ods in string related function
On Thursday 03 of January 2013, Markus Mohrhard wrote: Hey, while going through the list of calc documents crashing during import I came across gnome#627420-1.ods which creates an insanely large OUStringBuffer that ultimately leads to a crash. Since I believe we have quite a few places contain such problems. I wanted to ask if we should not try to find a solution in the string classes instead of having crashs with such documents from time to time. The question is, what kind of solution do you expect? Presumably the crash was because the allocation failed and the assert was a no-op because of non-debug build, leading to NULL pointer dereference. So probably the only thing we can do is have the assert always active, changing the crash to a different kind of crash, but that seems to be about it. The crash happens in: void SAL_CALL IMPL_RTL_STRINGNAME( new_WithLength )( IMPL_RTL_STRINGDATA** ppThis, sal_Int32 nLen ) SAL_THROW_EXTERN_C() with *ppThis = IMPL_RTL_STRINGNAME( ImplAlloc )( nLen ); OSL_ASSERT(*ppThis != NULL); (*ppThis)-length = 0; in the assignment of length to zero. -- Lubos Lunak l.lu...@suse.cz ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Crash with gnome#627420-1.ods in string related function
We could throw a StringAllocationFailed exception, and catch it near the bottom if the import process, and use that to indicate that the document is corrupt. On Thursday, 3 January 2013, Lubos Lunak wrote: On Thursday 03 of January 2013, Markus Mohrhard wrote: Hey, while going through the list of calc documents crashing during import I came across gnome#627420-1.ods which creates an insanely large OUStringBuffer that ultimately leads to a crash. Since I believe we have quite a few places contain such problems. I wanted to ask if we should not try to find a solution in the string classes instead of having crashs with such documents from time to time. The question is, what kind of solution do you expect? Presumably the crash was because the allocation failed and the assert was a no-op because of non-debug build, leading to NULL pointer dereference. So probably the only thing we can do is have the assert always active, changing the crash to a different kind of crash, but that seems to be about it. The crash happens in: void SAL_CALL IMPL_RTL_STRINGNAME( new_WithLength )( IMPL_RTL_STRINGDATA** ppThis, sal_Int32 nLen ) SAL_THROW_EXTERN_C() with *ppThis = IMPL_RTL_STRINGNAME( ImplAlloc )( nLen ); OSL_ASSERT(*ppThis != NULL); (*ppThis)-length = 0; in the assignment of length to zero. -- Lubos Lunak l.lu...@suse.cz javascript:; ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org javascript:; http://lists.freedesktop.org/mailman/listinfo/libreoffice ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Crash with gnome#627420-1.ods in string related function
2013/1/3 Lubos Lunak l.lu...@suse.cz: On Thursday 03 of January 2013, Markus Mohrhard wrote: Hey, while going through the list of calc documents crashing during import I came across gnome#627420-1.ods which creates an insanely large OUStringBuffer that ultimately leads to a crash. Since I believe we have quite a few places contain such problems. I wanted to ask if we should not try to find a solution in the string classes instead of having crashs with such documents from time to time. The question is, what kind of solution do you expect? Presumably the crash was because the allocation failed and the assert was a no-op because of non-debug build, leading to NULL pointer dereference. So probably the only thing we can do is have the assert always active, changing the crash to a different kind of crash, but that seems to be about it. Just to cear somethings up. I do have a dbgutil/debug build and it did not return a null pointer. It returned a pointer to some invalid memory which later results in the crash. Please also note that the document is perfectly fine and valid according to ODF and my fix for it is for example now only a random limit for the text length. I have no idea how to find a general solution but I know that it would be good to have a way to prevent crashes with these documents. It sounds much better to find a way to prevent the crash in these documents on the string implementation than hope to find all places in our import code. While it might be a good idea to fix the import and check the input we will always have new import code where we forget to add these additional dafety checks. A string that is several ten million characters long is a good indicator for a potential problem. Regards, Markus ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: Crash with gnome#627420-1.ods in string related function
2013/1/3 Noel Grandin noelgran...@gmail.com: We could throw a StringAllocationFailed exception, and catch it near the bottom if the import process, and use that to indicate that the document is corrupt. The document is not corrupt. IT is perfectly fine, the problem is either in our import code, which just misses sanity checks for quite a lot of input, or in the string class which can't handle with such situations at all. Since we have quite a lot of these places in the import code it might be better to find a solution in the string class.'m not sure if I'm not sure if throwing an exception is a good idea in this case as this might result in exceptions in constructors and even worse in destructors. It might be a solution to return an empty string or something like that in these cases or something like that and log it in a dbgutil build. ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
