802424 - netwerk/base/src/nsStandardURL.cpp(1535) : warning C4146: unary minus operator applied to unsigned type, result still unsigned

Status: RESOLVED WORKSFORME

(Core :: Networking, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This looks not good:

http://hg.mozilla.org/mozilla-central/annotate/838eee8853ca/netwerk/base/src/nsStandardURL.cpp#l1535

ehsan@102997  1531         uint32_t start = mHost.mPos + mHost.mLen;
ehsan@102997  1532         uint32_t lengthToCut = mPath.mPos - start;
bzbarsky@3548 1533         mSpec.Cut(start, lengthToCut);
bzbarsky@3548 1534         mAuthority.mLen -= lengthToCut;
bzbarsky@3548 1535         ShiftFromPath(-lengthToCut); 

Not sure what the security considerations are, if any, yet.

Comment 1

[Jeff Walden [:Waldo]]

Negation on an unsigned value equals (2**n - value), where n = width of value.  ShiftFromPath takes int32_t.  Technically super-large uint32_t converted to int32_t is undefined behavior.  In practice it's going to be twos-complement, and you're going to get the signed value you'd expect.  So I don't think there's anything dangerous here (assuming twos-complement).  Which isn't to say we shouldn't fix this by making the last -int32_t(lengthToCut), assuming we can guarantee lengthToCut will always fit in an int32_t.

I do so love how we have so many warnings we can randomly stumble across them in compiler output and go into panic mode over them.  :-\

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, comment 1 is spot on.  The code is safe as-is, but it would be better to cast before negating to avoid the warning.

That said, nothing per se guarantees that this thing will fit in an int32_t.  Maybe we should add a fatal abort in the case when it doesn't?  Chances of it happening are pretty rare, obviously, since they all involve a multi-gigabyte hostname.

Comment 3

[Patrick McManus [:mcmanus]]

valentin, something we can fixup and resolve?

Comment 4

[Valentin Gosu [:valentin] (he/him)]

That code is changed in attachment 8718741 [details] [diff] [review], which I reviewed recently.
Once it lands we can close it.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

This was fixed in bug 1247733 - landed in mozilla47.