Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(830)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 6733018: Use uint64_t and avoid down casting as much as possible. (Closed)

Created:
9 years, 9 months ago by gauravsh
Modified:
9 years, 6 months ago
Reviewers:
Randall Spangler, vb
CC:
chromium-os-reviews_chromium.org, Randall Spangler, gauravsh, Luigi Semenzato, Bill Richardson
Visibility:
Public.

Description

Use uint64_t and avoid down casting as much as possible. Change-Id: I231d1b3a059907c3806feced7e1b8f1c06575ba5 BUG=chromeos-partner:2912 TEST=make clean all && make runtests Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=d583a30

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -16 lines) Patch
M firmware/lib/cryptolib/include/rsa.h View 2 chunks +2 lines, -2 lines 0 comments Download
M firmware/lib/cryptolib/rsa_utility.c View 4 chunks +7 lines, -7 lines 0 comments Download
M firmware/lib/vboot_common.c View 1 chunk +5 lines, -5 lines 0 comments Download
M host/lib/host_key.c View 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
gauravsh
9 years, 9 months ago (2011-03-24 04:26:01 UTC) #1
gauravsh
9 years, 9 months ago (2011-03-24 04:26:10 UTC) #2
Randall Spangler
LGTM
9 years, 9 months ago (2011-03-24 04:35:44 UTC) #3
vb
just out of curiosity: what's the advantage of avoiding downcasting in this case - it ...
9 years, 9 months ago (2011-03-24 05:03:28 UTC) #4
gauravsh
9 years, 9 months ago (2011-03-24 06:13:11 UTC) #5 
On Mar 23, 2011 10:03 PM, "Vadim Bendebury" <vbendeb@chromium.org> wrote:
>
> just out of curiosity: what's the advantage of avoiding downcasting in
> this case - it is not obvious neither from CL description nor from the
> code changes...

Sorry, the bug referenced in the CL provides more context for the change.
Basically, we want to avoid translations between 64 and 32 bit ints to
prevent subtle overflow issues. It is less error prone to use 64 bit
everywhere instead of checking for possible  overflows for everytime we
downcast a 64 bit int to a 32 bit int.

>
> OTOH using 64 bit values on a 32 bit machine is slower
> (insignificantly, but still).

We are on a 64-bit machine, right. At least the firmware runs in 64 bit mode
even though our user space is 32 bit.
>
> cheers,
> /v
>
> On Wed, Mar 23, 2011 at 9:26 PM,  <gauravsh@chromium.org> wrote:
> > Reviewers: Randall,
> >
> > Description:
> > Use uint64_t and avoid down casting as much as possible.
> >
> > Change-Id: I231d1b3a059907c3806feced7e1b8f1c06575ba5
> >
> > BUG=chromeos-partner:2912
> > TEST=make clean all && make runtests
> >
> > Please review this at http://codereview.chromium.org/6733018/
> >
> > SVN Base: ssh://git@gitrw.chromium.org:9222/vboot_reference.git@master
> >
> > Affected files:
> >  M firmware/lib/cryptolib/include/rsa.h
> >  M firmware/lib/cryptolib/rsa_utility.c
> >  M firmware/lib/vboot_common.c
> >  M host/lib/host_key.c
> >
> >
> > Index: firmware/lib/cryptolib/include/rsa.h
> > diff --git a/firmware/lib/cryptolib/include/rsa.h
> > b/firmware/lib/cryptolib/include/rsa.h
> > index
> >
f5b83efaa157a920bacf585f44bcd9024846e9cb..eff608dced7200c460f318fa7bb5156c5f602a76
> > 100644
> > --- a/firmware/lib/cryptolib/include/rsa.h
> > +++ b/firmware/lib/cryptolib/include/rsa.h
> > @@ -75,7 +75,7 @@ int RSAVerifyBinaryWithDigest_f(const uint8_t*
key_blob,
> >  *
> >  * Returns 1 on success, 0 on failure.
> >  */
> > -int RSAProcessedKeySize(unsigned int algorithm, int* out_size);
> > +uint64_t RSAProcessedKeySize(uint64_t algorithm, uint64_t* out_size);
> >
> >  /* Allocate a new RSAPublicKey structure and initialize its pointer
fields
> > to
> >  * NULL */
> > @@ -89,7 +89,7 @@ void RSAPublicKeyFree(RSAPublicKey* key);
> >  *
> >  * Caller owns the returned key and must free it.
> >  */
> > -RSAPublicKey* RSAPublicKeyFromBuf(const uint8_t* buf, int len);
> > +RSAPublicKey* RSAPublicKeyFromBuf(const uint8_t* buf, uint64_t len);
> >
> >
> >  #endif  /* VBOOT_REFERENCE_RSA_H_ */
> > Index: firmware/lib/cryptolib/rsa_utility.c
> > diff --git a/firmware/lib/cryptolib/rsa_utility.c
> > b/firmware/lib/cryptolib/rsa_utility.c
> > index
> >
cd127107ee4b752066a2e70db419e9103e62fc9f..cc653c680528facac9735676b87539fceaf5ad7b
> > 100644
> > --- a/firmware/lib/cryptolib/rsa_utility.c
> > +++ b/firmware/lib/cryptolib/rsa_utility.c
> > @@ -9,9 +9,9 @@
> >  #include "stateful_util.h"
> >  #include "utility.h"
> >
> > -int RSAProcessedKeySize(unsigned int algorithm, int* out_size) {
> > -  int key_len; /* Key length in bytes. */
> > -  if (algorithm < (unsigned int)kNumAlgorithms) {
> > +uint64_t RSAProcessedKeySize(uint64_t algorithm, uint64_t* out_size) {
> > +  uint64_t key_len; /* Key length in bytes. */
> > +  if (algorithm < kNumAlgorithms) {
> >     key_len =  siglen_map[algorithm];
> >     /* Total size needed by a RSAPublicKey structure is =
> >      *  2 * key_len bytes for the  n and rr arrays
> > @@ -38,10 +38,10 @@ void RSAPublicKeyFree(RSAPublicKey* key) {
> >   }
> >  }
> >
> > -RSAPublicKey* RSAPublicKeyFromBuf(const uint8_t* buf, int len) {
> > +RSAPublicKey* RSAPublicKeyFromBuf(const uint8_t* buf, uint64_t len) {
> >   RSAPublicKey* key = RSAPublicKeyNew();
> >   MemcpyState st;
> > -  int key_len;
> > +  uint64_t key_len;
> >
> >   st.remaining_buf = (uint8_t*) buf;
> >   st.remaining_len = len;
> > @@ -81,7 +81,7 @@ int RSAVerifyBinary_f(const uint8_t* key_blob,
> >                       unsigned int algorithm) {
> >   RSAPublicKey* verification_key = NULL;
> >   uint8_t* digest = NULL;
> > -  int key_size;
> > +  uint64_t key_size;
> >   int sig_size;
> >   int success;
> >
> > @@ -120,7 +120,7 @@ int RSAVerifyBinaryWithDigest_f(const uint8_t*
key_blob,
> >                                 const uint8_t* sig,
> >                                 unsigned int algorithm) {
> >   RSAPublicKey* verification_key = NULL;
> > -  int key_size;
> > +  uint64_t key_size;
> >   int sig_size;
> >   int success;
> >
> > Index: firmware/lib/vboot_common.c
> > diff --git a/firmware/lib/vboot_common.c b/firmware/lib/vboot_common.c
> > index
> >
edc6e163b88b84163e2f5a1d6e446765f1898845..28d016ea1110b251f7b5dd8b3bcd392cc4d08cdf
> > 100644
> > --- a/firmware/lib/vboot_common.c
> > +++ b/firmware/lib/vboot_common.c
> > @@ -115,23 +115,23 @@ int PublicKeyCopy(VbPublicKey* dest, const
> > VbPublicKey* src) {
> >
> >  RSAPublicKey* PublicKeyToRSA(const VbPublicKey* key) {
> >   RSAPublicKey *rsa;
> > -  int key_size;
> > +  uint64_t key_size;
> >
> >   if (kNumAlgorithms <= key->algorithm) {
> >     VBDEBUG(("Invalid algorithm.\n"));
> >     return NULL;
> >   }
> > -  if (!RSAProcessedKeySize((int)key->algorithm, &key_size) ||
> > -      key_size != (int)key->key_size) {
> > +  if (!RSAProcessedKeySize(key->algorithm, &key_size) ||
> > +      key_size != key->key_size) {
> >     VBDEBUG(("Wrong key size for algorithm\n"));
> >     return NULL;
> >   }
> >
> > -  rsa = RSAPublicKeyFromBuf(GetPublicKeyDataC(key),
(int)key->key_size);
> > +  rsa = RSAPublicKeyFromBuf(GetPublicKeyDataC(key), key->key_size);
> >   if (!rsa)
> >     return NULL;
> >
> > -  rsa->algorithm = (int)key->algorithm;
> > +  rsa->algorithm = (unsigned int)key->algorithm;
> >   return rsa;
> >  }
> >
> > Index: host/lib/host_key.c
> > diff --git a/host/lib/host_key.c b/host/lib/host_key.c
> > index
> >
bcc89fcec0d3ac61ebaeaa776c1df00c4db2c0bc..33d911f24625e0c4e5febb2eccbad3c44d049cb0
> > 100644
> > --- a/host/lib/host_key.c
> > +++ b/host/lib/host_key.c
> > @@ -167,7 +167,7 @@ VbPublicKey* PublicKeyReadKeyb(const char* filename,
> > uint64_t algorithm,
> >   VbPublicKey* key;
> >   uint8_t* key_data;
> >   uint64_t key_size;
> > -  int expected_key_size;
> > +  uint64_t expected_key_size;
> >
> >   if (algorithm >= kNumAlgorithms) {
> >     VBDEBUG(("PublicKeyReadKeyb() called with invalid algorithm!\n"));
> > @@ -205,7 +205,7 @@ VbPublicKey* PublicKeyReadKeyb(const char* filename,
> > uint64_t algorithm,
> >  VbPublicKey* PublicKeyRead(const char* filename) {
> >   VbPublicKey* key;
> >   uint64_t file_size;
> > -  int key_size;
> > +  uint64_t key_size;
> >
> >   key = (VbPublicKey*)ReadFile(filename, &file_size);
> >   if (!key)
> >
> >
> >

Powered by Google App Engine
This is Rietveld 408576698