1534124 - Make nsMsgImapLineDownloadCache::SpaceAvailable() more robust

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1534119 +++

This is simply returning the difference of two unsigned ints which can be detrimental.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1534124-SpaceAvailable.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9049807 [details] [diff] [review]
1534124-SpaceAvailable.patch

Review of attachment 9049807 [details] [diff] [review]:
-----------------------------------------------------------------

I'm think this is not really optimal.
The assert could happen for a correct operation, so we shouldn't use assert for that.

Looks like kDownLoadCacheSize should always be unsigned, and handled as such.

Comment 3

[Jorg K (CEST = GMT+2)]

What? MOZ_ASSERT() only runs in debug so developers with a debug build can notice it.

If it happens for a "correct operation", which it shouldn't, I return 0, instead of returning -55 which is a huge number if taken unsigned.

I don't understand the last sentence, kDownLoadCacheSize is simply 16000:
https://searchfox.org/comm-central/rev/1e408f63a08ea6ba10bcabd906834edf9de4a7bb/mailnews/imap/src/nsImapProtocol.h#57

Comment 4

[Magnus Melin [:mkmelin]]

Assert is to notice programmatically wrong logic. In this the logic is correct I think, and could trigger with the suitable data amounts. That is, not a logic error.

I'm not sure about the syntax but perhaps "#define kDownLoadCacheSize 16000u" would give you what you want. That GrowBuffer uses a signed int looks wrong since it's cooperating with a cache that is unsigned.

Comment 5

[Jorg K (CEST = GMT+2)]

Right, assert is to detect wrong logic, so a pointer to after the end of a buffer seems to be as wrong as it gets.

That combined with then returning "-55 bytes available in the buffer" as a humongous amount being available is a total disaster. I don't need 16000u since I will only subtract a number less or equal to 16000.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1534124-SpaceAvailable.patch (v2)

This cleans up a bit more signed/unsigned confusion but won't make a difference.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9049980 [details] [diff] [review]
1534124-SpaceAvailable.patch (v2)

Let's have our backend specialist have a look ;-)

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9049980 [details] [diff] [review]
1534124-SpaceAvailable.patch (v2)

Review of attachment 9049980 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +224,5 @@
>  uint32_t nsMsgImapLineDownloadCache::SpaceAvailable()
>  {
> +  MOZ_ASSERT(kDownLoadCacheSize >= m_bufferPos);
> +  if (kDownLoadCacheSize <= m_bufferPos)
> +    return 0;

with the change to unsigned this shouldn't be necessary. The assert is slightly more correct in the new situation, but it's now an odd place to have it, so I'd rather skip it.

Comment 9

[Jorg K (CEST = GMT+2)]

I still don't understand. You can have two unsigned ints, like 16000u and 17000u and 16000u - 17000u surprisingly is a very large positive number. The original code just blindly subtracts two (now) unsigned numbers. What am I missing? How would you protect against a "bad" subtraction?

Comment 10

[Magnus Melin [:mkmelin]]

Ah I guess it's correct then.

Comment 11

[Ben Campbell]

Comment on attachment 9049980 [details] [diff] [review]
1534124-SpaceAvailable.patch (v2)

Review of attachment 9049980 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +224,5 @@
>  uint32_t nsMsgImapLineDownloadCache::SpaceAvailable()
>  {
> +  MOZ_ASSERT(kDownLoadCacheSize >= m_bufferPos);
> +  if (kDownLoadCacheSize <= m_bufferPos)
> +    return 0;

I think the assert is valid. The assert will not trigger because of malformed input data - it'll trigger because some code has screwed up the buffer and left it in an invalid state.

I'd suggest that `m_bufferSize` would be better to use than `kDownLoadCacheSize`, as it would remove the reliance on prior knowledge about calling `GrowBuffer(kDownloadCacheSize)`.

Is it a good idea returning 0 when things screw up in release builds? If a problem actually occurs, might this just mask it? I'd rather it just crashed hard then and there (and letting it return a negative number would probably do that nicely)

That said, I'm happy to r+ it as is. If the buffer does get into an odd state, the assert will pick it up in debugs build, and the release-build behaviour is going to be anomalous if that happens, safety-check or not.

Comment 12

[Jorg K (CEST = GMT+2)]

Thanks for looking at it so thoroughly. You mean m_dataBufferSize, right?
https://searchfox.org/comm-central/rev/cb674605dabf543dfb315457479b1e8186e2120c/mailnews/base/util/nsMsgLineBuffer.cpp#277

Comment 13

[Ben Campbell]

Ahh, yes, you're right - m_bufferSize is the one from nsByteArray!

Comment 14

[Jorg K (CEST = GMT+2)]

Well, sadly that doesn't compile since nsMsgImapLineDownloadCache can't access that member of nsMsgLineStreamBuffer even if you make if public :-(

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6117debc9c8a
Be more conservative in nsMsgImapLineDownloadCache::SpaceAvailable(). r=mkmelin,benc DONTBUILD

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Ben Campbell from comment #11)

Is it a good idea returning 0 when things screw up in release builds? If a
problem actually occurs, might this just mask it? I'd rather it just crashed
hard then and there (and letting it return a negative number would probably
do that nicely)

Well, there is no "negative" number, the negative number will be returned as a large positive number, so the code will happily overwrite the buffer and crash elsewhere in an uncontrolled fashion. The only option we have it to change the MOZ_ASSERT() to a MOZ_CRASH() or MOZ_RELEASE_ASSERT() but then the user crashes. It might be better to keep limping along with a truncated message. Hard to tell.