199056 - cookie_FindPosition() uses uninitialized oldestPositionFromHost; max cookies per host can be greater than 20 - Trunk [@ nsCString::~nsCString]

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[John Morrison]

0) For a given host, remove all cookies related to that host (e.g., using
   a test http server hostname)

1) Set 20 cookies for a host to expire in one year

  <script>
  window.onload = function () {
    for (var i = 0; i < 20; ++i) {
      var cookie = "x" + i + "=" + "x" + i + ";" + 
                   "expires=Wednesday, 24-Mar-2004 12:00:00 GMT";
      document.cookie = cookie;
    }
  };
  </script>

2) Set 20 more, different cookies for a host to expire in one year

  <script>
  window.onload = function () {
    for (var i = 0; i < 20; ++i) {
      var cookie = "y" + i + "=" + "y" + i + ";" + 
                   "expires=Wednesday, 24-Mar-2004 12:00:00 GMT";
      document.cookie = cookie;
    }
  };
  </script>

Result: 20 pairs of assertions like below

  ###!!! ASSERTION: nsVoidArray::ElementAt(index past end array) - note on 
      bug 96108: 'aIndex < Count()', file 
      h:/mozilla4/mozilla/xpcom/build/../ds\nsVoidArray.h, line 72
    Break: at file h:/mozilla4/mozilla/xpcom/build/../ds\nsVoidArray.h, line 72

  ###!!! ASSERTION: corrupt cookie list: 'deleteCookie', file 
      h:/mozilla4/mozilla/extensions/cookie/nsCookies.cpp, line 780
    Break: at file h:/mozilla4/mozilla/extensions/cookie/nsCookies.cpp, line 
780

3) quit and inspect the cookies.txt file; there are 40 cookies set for the host


The problem is that the variable 'oldestPositionFromHost' in
cookie_FindPosition() is used uninitialized.

Comment 1

[John Morrison]

This is wrong, wrong, wrong :-). We should fix this before 1.4a is done.

Comment 2

[John Morrison]

Or, just prepare a single file and run this script to set 40 cookies.

<script>
window.onload = function () {
    for (var i = 0; i < 20; ++i) {
        var cookie;
        cookie = "x" + i + "=" + "x" + i + ";" +
                 "expires=Wednesday, 24-Mar-2004 12:00:00 GMT";
        document.cookie = cookie;
        cookie = "y" + i + "=" + "y" + i + ";" +
                 "expires=Wednesday, 24-Mar-2004 12:00:00 GMT";
        document.cookie = cookie;
    }
};
</script>

Comment 3

[[e-mail]]

hmm, yes, this is wrong :)

however, I'm not immediately seeing how this is related to
oldestPositionFromHost begin uninitialized. i'm aware of that warning, and I've
deliberately ignored it because I think it's bogus (it's impossible for it to be
uninitialized if the precondition is satisfied; namely, countFromHost >=
MAX_COOKIES_PER_SERVER).

i'll look into this bug and get in a fix before 1.4a. thanks jrgm!

Comment 4

[[e-mail]]

*** Bug 198923 has been marked as a duplicate of this bug. ***

Comment 5

[[e-mail]]

okay, so this really had nothing to do with the uninited variable; that portion
is fine and i'll leave it that way (silly compilers)...

the problem is far more simple and embarassing. someone please r/sr and check
this in before anybody else notices ;)

Comment 6

[[e-mail]]

Attachment: fix

Comment 7

[[e-mail]]

Attachment: new fix; more context in diff

oops, forgot to fix another instance.

so the problem was in the comparison |if (oldestTime >
nsInt64(cookieInList->lastAccessed))| ... oldestTime has to be init'ed to
LL_MAXINT instead of LL_MININT, to ensure that the statement is executed at
least once.

ready for r/sr/checkin

Comment 8

[[e-mail]]

Comment on attachment 118440 [details] [diff] [review]
new fix; more context in diff

carrying over sr=heikki

Comment 9

[[e-mail]]

checked in by timeless

Comment 10

[John Morrison]

Actually, the warning was not bogus, and in fact was trying to tell you 
something important ;-)

Comment 11

[Jay Patel [:jay]]

Adding stack signature and crash, topcrash keywords for future reference (from
duped bug 198923).  This problem introduced a topcrasher.