mORMot Open Source

smetz84

Member

Registered: 2022-04-08

Posts: 3

Github pull requests

Hi guys,

I posted a pull request on the SynPDF GitHub project (https://github.com/synopse/SynPDF/pull/56) last year.
Is there a chance that this request will be reviewed one day?

Thanks

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,659

Website

Re: Github pull requests

I looked at it, and forgot to close it. Sorry.

Because there is no buffer overflow here. And your fix would make wrong results by the way.
Please look at the function use case (over a fixed size buffer of 256 bytes), and corresponding comment:

// we allow to copy up to 3/7 more chars in Dest^ since its size is 255

smetz84

Member

Registered: 2022-04-08

Posts: 3

Re: Github pull requests

Hi Arnaud,
Thank you for taking the time to review my request.

I'm not sure we are talking about the same thing. You're talking about the destination array, and I agree, buffer overflow is safe since sourceLen is maxed to 248 (although this is not 32-bit correct, but I will talk about it later (*)).
My pull request concern a buffer overrun on the source array in the case that sourceLen is a multiple a of 8 (64 bits) or 4 (32 bits). Let me give an example to illustrate it:

In 64 bits: if source sourceLen = 16, then sourceLen shr 3 = 2. So we'll iterate 3 times in the for loop (0 to 2), while we should iterate only 2 times (2 packets of 8 bytes), the 3rd iteration leads to a buffer overrun.
In 32 bits: if source sourceLen = 16, then sourceLen shr 2 = 4. So we'll iterate 5 times in the for loop (0 to 4), while we should iterate only 4 times (4 packets of 4 bytes), the 5th iteration leads to a buffer overrun.

I introduced this correction in my project because a large number of my users reported crashes at this place, since the fix, I no longer have any problems.

(*) When sourceLen is maxed to 248, it sounds good for the 64 bits version, but in the 32 bits version, if sourceLen > 252, the 4 last characters will be ignored and not processed.

Last edited by smetz84 (2022-04-11 09:50:51)

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,659

Website

Re: Github pull requests

About (sourceLen - 1), I now understand what you mean.
So your pull request seems correct, better than my initial code.
It was a classic mistake of mine, from a "boundary count" issue.
Should be fixed by https://github.com/synopse/mORMot2/commit/cfb47b84
and https://github.com/synopse/mORMot/commi … 0faff616b2

The idea about maximum input process of 248 is that the input is truncated to its first chars, without writing outside the buffer.
Either it is no problem (e.g. before hashing, since we allow collisions), or it will lead to less chars tested (e.g. before an INI key value search), but then it will behave the same on 32-bit and 64-bit, and the limitation to 248 max number of chars is documented as such. If you have an INI key bigger than 248 chars, then it will fail. But the key is clearly too large to be usable. So this is documented as a known limitation.

smetz84

Member

Registered: 2022-04-08

Posts: 3

Re: Github pull requests

Thank you for the fix.

According the 248 max value, I just analyzed the situation from an algorithmic point of view in the scope of the function, but if this is a known limitation documented as such, that's fine.