327161 - nsUUIDGenerator subject to reseeding woes

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

nsUUIDGenerator uses random(), which it seeds once (at init time).  Unfortunately, there are lots of other uses of rand/srand (and a few of random/srandom), and I wouldn't rely on people not seeding with the same thing over and over.  Which would be bad, since it would cause duplicate IIDs.

Perhaps we should reseed for each get of a uuid?  Possibly using the last returned thing as the seed or something?

I'd really like this for bug 326506.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Reseeding sounds fine to me; it would be even better if we had a prng with local state we could use.. not sure if NSS provides anything that could help.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Kai, do you know what NSS/PSM provide for us here?

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

isn't uuidgenerator in XPCOM?

Comment 4

[Benjamin Smedberg]

Since this implementation lives in XPCOM, NSS cannot provide anything directly (other than source code).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ugh.  And NSPR certainly doesn't provide anything... yet.

Comment 6

[Darin Fisher]

XPCOM could poke an API that it defines that PSM implements.

Comment 7

[Boris Zbarsky [:bzbarsky]]

We're using this for security stuff on trunk now, so we really need to get this done before we ship Gecko 1.9.

Comment 8

[Boris Zbarsky [:bzbarsky]]

It looks like bug 279521 landed on branch, so we need to get this fixed there too, imo...

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: always restore our own private RNG state

Save/restore the RNG state we initialize with initstate around each call.  (Tested this with explicitly putting a srandom(5) before the setstate call in generateUUID, seems to work.)

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233595 [details] [diff] [review]
always restore our own private RNG state

Looks good!  Thanks!

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 233595 [details] [diff] [review]
always restore our own private RNG state

Requesting a1.8.1; simple patch, avoids reseeding issues with uuid generator.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checking in nsUUIDGenerator.cpp;
/cvsroot/mozilla/xpcom/base/nsUUIDGenerator.cpp,v  <--  nsUUIDGenerator.cpp
new revision: 1.7; previous revision: 1.6
done

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: attempt to fix btek

Attempt to fix btek; I think this should initialize the internal state buffer so that initstate() doesn't return NULL (and so that setstate doesn't crash when we pass in NULL).

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233648 [details] [diff] [review]
attempt to fix btek

Let's give it a shot!

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Note that this caused btek orange due to old glibc, but I'm trying to figure out whether it's a btek problem or if newer glibc is just masking another problem.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: real fix for btek

Real fix for btek, tested live on btek already.  Apparently btek's glibc has a bug in the return value from setstate().. it seems to return bogus values.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: branch patch, with above goop combined

Branch patch with combination of above stuff, ready for checkin once approved tomorrow

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233701 [details] [diff] [review]
branch patch, with above goop combined

a=beltzner on behalf of drivers for the mozilla_1_8_branch

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in on 1.8.1.

Comment 20

[Sergei Dolgov]

I think it is not so good idea to allow exit nsUUIDGenerator::Init()
with mSavedState uninitialized:
http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsUUIDGenerator.cpp#88
even if we return NS_ERROR_FAILURE.

See
https://bugzilla.mozilla.org/show_bug.cgi?id=349526#c40