Re: [REVIEW-3.5] crasher fdo#45987
Hello Noel, Kohei, I think [1] is a better patch for 3-5. I'm still not happy with it because I had hoped that we could init the number of sheets already in the constructor but this works only while loading a file and not when we create an empty one. This hack is a bit dirty but was the only useful place that already knows about the number of sheets and does not force us to create some more ugly hacks. It just makes sure that the number of sheets in ScViewData and ScDocument is the same. It is a safe fix and we can include it in 3-5. Regards, Markus [1] http://cgit.freedesktop.org/libreoffice/core/commit/?id=13bb6dcddcd3e19cd97fdb28e842c778e6e093b3 ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [REVIEW-3.5] crasher fdo#45987
On 18/05/12 14:49, Markus Mohrhard wrote: Hey Noel 2012/5/18 Kohei Yoshida: On Fri, May 18, 2012 at 9:14 AM, Noel Power wrote: http://cgit.freedesktop.org/libreoffice/core/commit/?id=8b1d29bc9b00bc2730738a990023a65ab6e0219b & http://cgit.freedesktop.org/libreoffice/core/commit/?id=abb26f51eea0399754cc8f5b7d7a7d648d68f630 I took it that it should work how I outlined above and committed a further fix which should safeguard against illegal access, please additionally consider http://cgit.freedesktop.org/libreoffice/core/commit/?id=8352eb5a1af1eb44550a9d60d31e6c2fb2dc43b9 So, these extra range checks should be safe; however The original intention of *not* checking the bound of the tab index was that we assumed that this list would be in sync with the maTab in ScDocument at all times. So, if they are out of sync then something else may be causing this problem... Let me ping Markus here. He worked on the rework of ScTable storage, and this issue is related to that work. I agree with Kohei. Needing a range check at this place will most likely hide a underlying problem. The table container in ScViewData and the one in ScDocument must always be in sync. I fear that this might have been a problem that has always been there. Loosing the sync between the two data structures will result in wrong sheet numbers coming from the view part. Let me check this bug report as soon as I have a clean tree. ok I might have jumped the gun ( should have waited for the answer ;-) ), fair enough, feel free to revert those commits in master then. Probably the ViewData should be initialised with the same number of Tabs as the document, being the view doesn't it just track what views have been selected ? ( so probably it has just 1 setup initially e.g. the current view ) saying that though ( now that I think of it I could swear that in fact there were the size of the maTabData was 2 in the bug scenario which is strange, maybe some over zealous allocation ? ) Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [REVIEW-3.5] crasher fdo#45987
Hey Noel 2012/5/18 Kohei Yoshida : > On Fri, May 18, 2012 at 9:14 AM, Noel Power wrote: > >>> http://cgit.freedesktop.org/libreoffice/core/commit/?id=8b1d29bc9b00bc2730738a990023a65ab6e0219b >>> & >>> >>> http://cgit.freedesktop.org/libreoffice/core/commit/?id=abb26f51eea0399754cc8f5b7d7a7d648d68f630 >> >> >> I took it that it should work how I outlined above and committed a further >> fix which should safeguard against illegal access, please additionally >> consider >> http://cgit.freedesktop.org/libreoffice/core/commit/?id=8352eb5a1af1eb44550a9d60d31e6c2fb2dc43b9 > > So, these extra range checks should be safe; however The original > intention of *not* checking the bound of the tab index was that we > assumed that this list would be in sync with the maTab in ScDocument > at all times. So, if they are out of sync then something else may be > causing this problem... > > Let me ping Markus here. He worked on the rework of ScTable storage, > and this issue is related to that work. I agree with Kohei. Needing a range check at this place will most likely hide a underlying problem. The table container in ScViewData and the one in ScDocument must always be in sync. I fear that this might have been a problem that has always been there. Loosing the sync between the two data structures will result in wrong sheet numbers coming from the view part. Let me check this bug report as soon as I have a clean tree. Regards, Markus ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [REVIEW-3.5] crasher fdo#45987
On Fri, May 18, 2012 at 9:14 AM, Noel Power wrote: >> http://cgit.freedesktop.org/libreoffice/core/commit/?id=8b1d29bc9b00bc2730738a990023a65ab6e0219b >> & >> >> http://cgit.freedesktop.org/libreoffice/core/commit/?id=abb26f51eea0399754cc8f5b7d7a7d648d68f630 > > > I took it that it should work how I outlined above and committed a further > fix which should safeguard against illegal access, please additionally > consider > http://cgit.freedesktop.org/libreoffice/core/commit/?id=8352eb5a1af1eb44550a9d60d31e6c2fb2dc43b9 So, these extra range checks should be safe; however The original intention of *not* checking the bound of the tab index was that we assumed that this list would be in sync with the maTab in ScDocument at all times. So, if they are out of sync then something else may be causing this problem... Let me ping Markus here. He worked on the rework of ScTable storage, and this issue is related to that work. Kohei ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [REVIEW-3.5] crasher fdo#45987
On 18/05/12 10:31, Noel Power wrote: On 18/05/12 10:26, Noel Power wrote: Hi somewhat of a containing fix ( I think there is other rework needed, see below ) that avoids the mentioned crasher, please consider cherry-picking to 3.5 Always ( afaict ) the code expects the index of the entry in the (maTabs) vector to correspond a tab of the same index. However the DeleteTab routine patched above will erase the entry for the tab but if that tab isn't the last tab but instead some random tab in the middle won't the order of the tabs be screwed ? We could check and only delete an entry if it is the last entry ( and otherwise make a null entry for the deleted tab ) but then we definitely would expect the code should be ready to deal with such a 'hole' (representing a sheet that no longer exists ) in the vector, that doesn't appear to be the case. Is this how it should work ? I could rework it like that if that is the intention, is it ? and I forgot to mention the commit ( or actually 2 commit ids as it appears I introduced a wae that was fixed by sb ) http://cgit.freedesktop.org/libreoffice/core/commit/?id=8b1d29bc9b00bc2730738a990023a65ab6e0219b & http://cgit.freedesktop.org/libreoffice/core/commit/?id=abb26f51eea0399754cc8f5b7d7a7d648d68f630 I took it that it should work how I outlined above and committed a further fix which should safeguard against illegal access, please additionally consider http://cgit.freedesktop.org/libreoffice/core/commit/?id=8352eb5a1af1eb44550a9d60d31e6c2fb2dc43b9 thanks, Noel ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [REVIEW-3.5] crasher fdo#45987
On 18/05/12 10:26, Noel Power wrote: Hi somewhat of a containing fix ( I think there is other rework needed, see below ) that avoids the mentioned crasher, please consider cherry-picking to 3.5 Always ( afaict ) the code expects the index of the entry in the (maTabs) vector to correspond a tab of the same index. However the DeleteTab routine patched above will erase the entry for the tab but if that tab isn't the last tab but instead some random tab in the middle won't the order of the tabs be screwed ? We could check and only delete an entry if it is the last entry ( and otherwise make a null entry for the deleted tab ) but then we definitely would expect the code should be ready to deal with such a 'hole' (representing a sheet that no longer exists ) in the vector, that doesn't appear to be the case. Is this how it should work ? I could rework it like that if that is the intention, is it ? and I forgot to mention the commit ( or actually 2 commit ids as it appears I introduced a wae that was fixed by sb ) http://cgit.freedesktop.org/libreoffice/core/commit/?id=8b1d29bc9b00bc2730738a990023a65ab6e0219b & http://cgit.freedesktop.org/libreoffice/core/commit/?id=abb26f51eea0399754cc8f5b7d7a7d648d68f630 ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[REVIEW-3.5] crasher fdo#45987
Hi somewhat of a containing fix ( I think there is other rework needed, see below ) that avoids the mentioned crasher, please consider cherry-picking to 3.5 Always ( afaict ) the code expects the index of the entry in the (maTabs) vector to correspond a tab of the same index. However the DeleteTab routine patched above will erase the entry for the tab but if that tab isn't the last tab but instead some random tab in the middle won't the order of the tabs be screwed ? We could check and only delete an entry if it is the last entry ( and otherwise make a null entry for the deleted tab ) but then we definitely would expect the code should be ready to deal with such a 'hole' (representing a sheet that no longer exists ) in the vector, that doesn't appear to be the case. Is this how it should work ? I could rework it like that if that is the intention, is it ? ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
