Re: [PATCH 2/3] storage.dll16: Correctly handle block values of -1.

[Alexandre Julliard]

Zebediah Figura  writes:
> @@ -610,9 +610,10 @@ STORAGE_get_nth_next_small_blocknr(stream_access16*str,int blocknr,int nr) {
>    READ_HEADER(str);
>    assert(blocknr>=0);
>    while ((nr--) && (blocknr>=0)) {
> -
> if (lastblocknr/128!=blocknr/128) {
> -
> if (lastblocknr>>7 != blocknr>>7)
> -
> {
> int	bigblocknr;
> -
> 	bigblocknr = STORAGE_get_nth_next_big_blocknr(str,sth.sbd_startblock,blocknr/128);
> -
> 	bigblocknr = STORAGE_get_nth_next_big_blocknr(str,sth.sbd_startblock,blocknr>>7);

I'm not sure what you are trying to fix. Most places already check that
blocknr is >= 0, and if it's really negative, you can't rely on right
shift doing the right thing.

[PATCH 2/3] storage.dll16: Correctly handle block values of -1.

[Zebediah Figura]

On 02/20/2017 03:39 AM, Alexandre Julliard wrote:
> Zebediah Figura  writes:
> > @@ -610,9 +610,10 @@ STORAGE_get_nth_next_small_blocknr(stream_access16*str,int blocknr,int nr) {
> >    READ_HEADER(str);
> >    assert(blocknr>=0);
> >    while ((nr--) && (blocknr>=0)) {
> > -
> > if (lastblocknr/128!=blocknr/128) {
> > -
> > if (lastblocknr>>7 != blocknr>>7)
> > -
> > {
> > int	bigblocknr;
> > -
> > 	bigblocknr = STORAGE_get_nth_next_big_blocknr(str,sth.sbd_startblock,blocknr/128);
> > -
> > 	bigblocknr = STORAGE_get_nth_next_big_blocknr(str,sth.sbd_startblock,blocknr>>7);
>
> I'm not sure what you are trying to fix. Most places already check that
> blocknr is >= 0, and if it's really negative, you can't rely on right
> shift doing the right thing.

Without the patch, I get failed assertions in several places that the
blocknr is >= 0. With it, it seems to pass that -1 on to the program,
causing it not to crash. This might be fixing the symptom rather than
the cause, though.

[PATCH 2/3] storage.dll16: Correctly handle block values of -1.

[Zebediah Figura]

On 02/20/2017 08:23 AM, Zebediah Figura wrote:
> On 02/20/2017 03:39 AM, Alexandre Julliard wrote:
> > Zebediah Figura  writes:
> > > @@ -610,9 +610,10 @@ STORAGE_get_nth_next_small_blocknr(stream_access16*str,int blocknr,int nr) {
> > >    READ_HEADER(str);
> > >    assert(blocknr>=0);
> > >    while ((nr--) && (blocknr>=0)) {
> > > -
> > > if (lastblocknr/128!=blocknr/128) {
> > > -
> > > if (lastblocknr>>7 != blocknr>>7)
> > > -
> > > {
> > > int	bigblocknr;
> > > -
> > > 	bigblocknr = STORAGE_get_nth_next_big_blocknr(str,sth.sbd_startblock,blocknr/128);
> > > -
> > > 	bigblocknr = STORAGE_get_nth_next_big_blocknr(str,sth.sbd_startblock,blocknr>>7);
> >
> > I'm not sure what you are trying to fix. Most places already check that
> > blocknr is >= 0, and if it's really negative, you can't rely on right
> > shift doing the right thing.
>
> Without the patch, I get failed assertions in several places that the
> blocknr is >= 0. With it, it seems to pass that -1 on to the program,
> causing it not to crash. This might be fixing the symptom rather than
> the cause, though.

My apologies. To be more accurate, the game crashes because this
function is passed blocknr=12, nr=1, so when the loop is executed,
lastblocknr/128 == blocknr/128 == 0, so the assertion "lastblocknr >= 0"
fails. If there is a better way to fix this, I do not know what it is.