ronantakizawa/codereview-bench huggingface dataset:
Repository: google/OpenSK
File: libraries/persistent_store/src/storage.rs (Rust)

Code before:
//! Flash storage abstraction.

use alloc::borrow::Cow;

/// Represents a byte position in a storage.
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct StorageIndex {
    pub page: usize,
    pub byte: usize,
}

/// Represents a possible storage error.
#[derive(Debug, PartialEq, Eq)]
pub enum StorageError {
    /// Arguments are not correctly aligned.
    NotAligned,

    /// Arguments are out of bounds.
    OutOfBounds,

    /// Implementation-specific error.
    CustomError,

    // I/O error
    #[cfg(feature = "hostenv")]
    IOError,
}

#[cfg(feature = "hostenv")]
#[allow(unused_variables)]
impl From<std::io::Error> for StorageError {
    fn from(error: std::io::Error) -> Self {
        Self::IOError
    }
}

pub type StorageResult<T> = Result<T, StorageError>;

/// Abstracts a flash storage.
pub trait Storage {
    /// The size of a word in bytes.
    ///
    /// A word is the smallest unit of writable flash.
    fn word_size(&self) -> usize;

    /// The size of a page in bytes.
    ///
    /// A page is the smallest unit of erasable flash.
    fn page_size(&self) -> usize;

    /// The number of pages in the storage.

Review comment (suggestion):
Let's use `CustomError` instead. It's the error for implementation-specific errors, like IO errors in this case.

Code after:
//! Flash storage abstraction.

use alloc::borrow::Cow;

/// Represents a byte position in a storage.
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct StorageIndex {
    pub page: usize,
    pub byte: usize,
}

/// Represents a possible storage error.
#[derive(Debug, PartialEq, Eq)]
pub enum StorageError {
    /// Arguments are not correctly aligned.
    NotAligned,

    /// Arguments are out of bounds.
    OutOfBounds,

    /// Implementation-specific error.
    CustomError,
}

#[cfg(feature = "std")]
impl From<std::io::Error> for StorageError {
    fn from(_: std::io::Error) -> Self {
        Self::CustomError
    }
}

pub type StorageResult<T> = Result<T, StorageError>;

/// Abstracts a flash storage.
pub trait Storage {
    /// The size of a word in bytes.
    ///
    /// A word is the smallest unit of writable flash.
    fn word_size(&self) -> usize;

    /// The size of a page in bytes.
    ///
    /// A page is the smallest unit of erasable flash.
    fn page_size(&self) -> usize;

    /// The number of pages in the storage.
    fn num_pages(&self) -> usize;

    /// Maximum number of times a word can be written between page erasures.
    fn max_word_writes(&self) -> usize;
Repository: google/OpenSK
File: libraries/persistent_store/src/file.rs (Rust)

Code before:
    /// Size of a word in bytes.
    pub word_size: usize,

    /// Size of a page in bytes.
    pub page_size: usize,

    /// Number of pages in storage.
    pub num_pages: usize,
}

impl FileStorage {
    pub fn new(path: &Path, options: FileOptions) -> StorageResult<FileStorage> {
        let mut file_ref = RefCell::new(
            OpenOptions::new()
                .read(true)
                .write(true)
                .create(true)
                .open(path)?,
        );
        let file = file_ref.get_mut();
        let file_len = file.metadata()?.len();
        let store_len: u64 = (options.page_size * options.num_pages) as u64;

        if file_len == 0 {
            file.seek(SeekFrom::Start(0))?;
            let buf = vec![0xffu8; options.page_size];
            for _ in 0..options.num_pages {
                file.write(&buf)?;
            }
        } else if file_len != store_len {
            // FileStorage buffer should be of fixed size, opening previously saved file
            // from storage of different size is not supported
            panic!("Invalid file size {}, should be {}", file_len, store_len);
        }
        Ok(FileStorage {
            options,
            file: file_ref,
        })
    }
}

impl Storage for FileStorage {
    fn word_size(&self) -> usize {
        self.options.word_size
    }

    fn page_size(&self) -> usize {
        self.options.page_size
    }

    fn num_pages(&self) -> usize {

Review comment (suggestion):
We have generally used `0xFF`. Is the `u8` annotation is required?

Other instances of this exist below.

Code after:
    /// Size of a word in bytes.
    pub word_size: usize,

    /// Size of a page in bytes.
    pub page_size: usize,

    /// Number of pages in storage.
    pub num_pages: usize,
}

impl FileStorage {
    pub fn new(path: &Path, options: FileOptions) -> StorageResult<FileStorage> {
        let mut file_ref = RefCell::new(
            OpenOptions::new()
                .read(true)
                .write(true)
                .create(true)
                .open(path)?,
        );
        let file = file_ref.get_mut();
        let file_len = file.metadata()?.len();
        let store_len: u64 = (options.page_size * options.num_pages) as u64;

        if file_len == 0 {
            file.seek(SeekFrom::Start(0))?;
            let buf = vec![0xff; options.page_size];
            for _ in 0..options.num_pages {
                file.write(&buf)?;
            }
        } else if file_len != store_len {
            // FileStorage buffer should be of fixed size, opening previously saved file
            // from storage of different size is not supported
            panic!("Invalid file size {}, should be {}", file_len, store_len);
        }
        Ok(FileStorage {
            options,
            file: file_ref,
        })
    }
}

impl Storage for FileStorage {
    fn word_size(&self) -> usize {
        self.options.word_size
    }

    fn page_size(&self) -> usize {
        self.options.page_size
    }

    fn num_pages(&self) -> usize {
Repository: google/OpenSK
File: tools/vendor_hid_test.py (Python)

Code before:
_OPENSK_VID = 0x1915
_OPENSK_PID = 0x521F
_FIDO_USAGE_PAGE = 0xF1D0
_VENDOR_USAGE_PAGE = 0xFF00
_PACKETS = 4
_PACKET_SIZE = 64
_SEND_DATA_SIZE = _PACKET_SIZE + 1
_DEFAULT_CID = bytes([0xFF, 0xFF, 0xFF, 0xFF])


def sleep():
  #time.sleep(.01)
  pass


def ping_data_size(packets):
  return 57 + 59 * (packets - 1)


_BYTE_VAL = 0x10


def get_byte():
  global _BYTE_VAL
  _BYTE_VAL = _BYTE_VAL + 1
  return _BYTE_VAL


class HidDevice(object):
  """Class that helps interact with HID devices."""

  def __init__(self, device):
    self.device = device
    self.dev = None
    self.cid = None
    self.rx_packets = []
    self.create_and_init()

  def __del__(self):
    if self.dev:
      self.dev.close()

  def create_and_init(self) -> None:
    self.dev = hid.Device(path=self.device['path'])
    # Nonce is all zeros, because we don't care.
    init_packet = [0] + list(_DEFAULT_CID) + [0x86, 0x00, 0x08] + [0x00] * 57
    if len(init_packet) != _SEND_DATA_SIZE:
      raise Exception(
          f'Expected packet to be {_SEND_DATA_SIZE} but was {len(init_packet)}')
    self.dev.write(bytes(init_packet))
    self.cid = self.dev.read(_PACKET_SIZE, 2000)[15:19]

Review comment (suggestion):
Since `_BYTE_VAL` is technically local to the function, we could also do this:
```
def get_byte():
  get_byte.byte_val += 1
  return get_byte.byte_val
get_byte.byte_val = 0x10
```

I wonder if that's worth the trouble. Sending a const per endpoint would also work for me.

Code after:

_OPENSK_VID = 0x1915
_OPENSK_PID = 0x521F
_FIDO_USAGE_PAGE = 0xF1D0
_VENDOR_USAGE_PAGE = 0xFF00
_PACKETS = 4
_PACKET_SIZE = 64
_SEND_DATA_SIZE = _PACKET_SIZE + 1
_BROADCAST_CID = bytes([0xFF, 0xFF, 0xFF, 0xFF])


def sleep():
  # TODO(liamjm): remove this sleep once it is not necessary.
  time.sleep(.01)
  pass


def ping_data_size(packets):
  return 57 + 59 * (packets - 1)


def get_byte():
  """Return a unique byte value per request."""
  get_byte.byte_val += 1
  return get_byte.byte_val


get_byte.byte_val = 0x10


class HidDevice(object):
  """Class that helps interact with HID devices."""

  def __init__(self, device):
    self.device = device
    self.dev = None
    self.cid = None
    self.rx_packets = []
    self.create_and_init()

  def __del__(self):
    if self.dev:
      self.dev.close()

  def reset(self) -> None:
    self.rx_packets = []

  def create_and_init(self) -> None:
    self.dev = hid.Device(path=self.device['path'])
    # Nonce is all zeros, because we don't care.
    init_packet = [0] + list(_BROADCAST_CID) + [0x86, 0x00, 0x08] + [0x00] * 57
Repository: google/OpenSK
File: src/api/attestation_store.rs (Rust)

Code before:
use alloc::string::String;
use alloc::vec::Vec;
use persistent_store::{StoreError, StoreUpdate};

use crate::env::Env;

/// Identifies an attestation.
pub enum Id {
    Batch,
    Enterprise { rp_id: String },
}

#[cfg_attr(feature = "std", derive(Debug, PartialEq, Eq))]
pub struct Attestation {
    /// ECDSA private key (big-endian).
    pub private_key: [u8; 32],
    pub certificate: Vec<u8>,
}

/// Stores enterprise or batch attestations.
///
/// Implementations don't need to distinguish different attestations. In particular, setting one
/// attestation may set other ones.
pub trait AttestationStore {
    /// Returns an attestation given its id, if it exists.
    ///
    /// This should always return the attestation. Checking whether it is ok to use the attestation
    /// is done in the CTAP library.
    fn get(&mut self, id: &Id) -> Result<Option<Attestation>, Error>;

    /// Sets the attestation for a given id.
    ///
    /// This function may not be supported.
    fn set(&mut self, id: &Id, attestation: Option<&Attestation>) -> Result<(), Error>;
}

/// Attestation store errors.
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
#[non_exhaustive]
pub enum Error {
    Storage,
    Internal,
    NoSupport,
}

/// Keys of the environment store reserved for the attestation store.
pub const STORAGE_KEYS: &[usize] = &[1, 2];


Review comment (performance):
We can actually remove this if for the default implementation we decide to only support batch attestation. I think it makes sense because if someone wants to customize with enterprise attestation, they would have a custom implementation anyway. What do you think?

Code after:
use alloc::vec::Vec;
use persistent_store::{StoreError, StoreUpdate};

use crate::env::Env;

/// Identifies an attestation.
#[derive(Clone, PartialEq, Eq)]
pub enum Id {
    Batch,
    Enterprise,
}

#[cfg_attr(feature = "std", derive(Debug, PartialEq, Eq))]
pub struct Attestation {
    /// ECDSA private key (big-endian).
    pub private_key: [u8; 32],
    pub certificate: Vec<u8>,
}

/// Stores enterprise or batch attestations.
pub trait AttestationStore {
    /// Returns an attestation given its id, if it exists.
    ///
    /// This should always return the attestation. Checking whether it is ok to use the attestation
    /// is done in the CTAP library.
    fn get(&mut self, id: &Id) -> Result<Option<Attestation>, Error>;

    /// Sets the attestation for a given id.
    ///
    /// This function may not be supported.
    fn set(&mut self, id: &Id, attestation: Option<&Attestation>) -> Result<(), Error>;
}

/// Attestation store errors.
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
#[non_exhaustive]
pub enum Error {
    Storage,
    Internal,
    NoSupport,
}

/// Keys of the environment store reserved for the attestation store.
pub const STORAGE_KEYS: &[usize] = &[1, 2];

pub fn helper_get(env: &mut impl Env) -> Result<Option<Attestation>, Error> {
    let private_key = env.store().find(PRIVATE_KEY_STORAGE_KEY)?;
    let certificate = env.store().find(CERTIFICATE_STORAGE_KEY)?;
Repository: google/OpenSK
File: src/api/attestation_store.rs (Rust)

Code before:
use alloc::string::String;
use alloc::vec::Vec;
use persistent_store::{StoreError, StoreUpdate};

use crate::env::Env;

/// Identifies an attestation.
pub enum Id {
    Batch,
    Enterprise { rp_id: String },
}

#[cfg_attr(feature = "std", derive(Debug, PartialEq, Eq))]
pub struct Attestation {
    /// ECDSA private key (big-endian).
    pub private_key: [u8; 32],
    pub certificate: Vec<u8>,
}

/// Stores enterprise or batch attestations.
///
/// Implementations don't need to distinguish different attestations. In particular, setting one
/// attestation may set other ones.
pub trait AttestationStore {
    /// Returns an attestation given its id, if it exists.
    ///
    /// This should always return the attestation. Checking whether it is ok to use the attestation
    /// is done in the CTAP library.
    fn get(&mut self, id: &Id) -> Result<Option<Attestation>, Error>;

    /// Sets the attestation for a given id.
    ///
    /// This function may not be supported.
    fn set(&mut self, id: &Id, attestation: Option<&Attestation>) -> Result<(), Error>;
}

Review comment (suggestion):
Let's remove the `rp_id`, I don't think we have use cases for different enterprise attestations at the same time.

Code after:
use alloc::vec::Vec;
use persistent_store::{StoreError, StoreUpdate};

use crate::env::Env;

/// Identifies an attestation.
#[derive(Clone, PartialEq, Eq)]
pub enum Id {
    Batch,
    Enterprise,
}

#[cfg_attr(feature = "std", derive(Debug, PartialEq, Eq))]
pub struct Attestation {
    /// ECDSA private key (big-endian).
    pub private_key: [u8; 32],
    pub certificate: Vec<u8>,
}

/// Stores enterprise or batch attestations.
pub trait AttestationStore {
    /// Returns an attestation given its id, if it exists.
    ///
    /// This should always return the attestation. Checking whether it is ok to use the attestation
    /// is done in the CTAP library.
    fn get(&mut self, id: &Id) -> Result<Option<Attestation>, Error>;

    /// Sets the attestation for a given id.
    ///
    /// This function may not be supported.
    fn set(&mut self, id: &Id, attestation: Option<&Attestation>) -> Result<(), Error>;
}

/// Attestation store errors.
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
Repository: google/OpenSK
File: tools/perform_upgrade.sh (Shell)

Code before:
#!/bin/bash
# Copyright 2021 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#      http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# Creates a signature key and configures the public key.
# The device will not be locked down for testing purposes.
# Generates the binary and upgrades OpenSK.
# To be run from the OpenSK base path.

set -e

BOARD="$1"

./deploy.py --board=$BOARD --opensk --programmer=none
python3 -m tools.deploy_partition --board=$BOARD "$2"
if nrfjprog --reset --family NRF52 ; then
  echo "Upgrade finished!"
else
  echo "Please replug OpenSK to reboot"
fi


Review comment (suggestion):
nit: This is not very clean, but if python accepts empty arguments then why not. I see at least 2 solutions:

```
BOARD="$1"
shift

./deploy.py ...
python3 -m ... --board="${BOARD}" "$@"
```
(note that Google style for bash is `"${var}"`, same for deploy.py line)

```
python3 ... --board="${BOARD}" $2
```
(note the absence of double quotes, this means that if $2 is empty, no extra argument is passed, while double quotes would pass an empty argument)

Code after:
#!/bin/bash
# Copyright 2021 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#      http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# Creates a signature key and configures the public key.
# The device will not be locked down for testing purposes.
# Generates the binary and upgrades OpenSK.
# To be run from the OpenSK base path.

set -e

BOARD="$1"

./deploy.py --board="${BOARD}" --opensk --programmer=none $2
python3 -m tools.deploy_partition --board="${BOARD}" $2
if nrfjprog --reset --family NRF52 ; then
  echo "Upgrade finished!"
else
  echo "Please replug OpenSK to reboot"
fi
Repository: google/OpenSK
File: tools/perform_upgrade.sh (Shell)

Code before:
#!/bin/bash
# Copyright 2021 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#      http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# Creates a signature key and configures the public key.
# The device will not be locked down for testing purposes.
# Generates the binary and upgrades OpenSK.
# To be run from the OpenSK base path.

set -e

BOARD="$1"

./deploy.py --board=${BOARD} --opensk --programmer=none $2
python3 -m tools.deploy_partition --board="${BOARD}" $2
if nrfjprog --reset --family NRF52 ; then
  echo "Upgrade finished!"
else
  echo "Please replug OpenSK to reboot"
fi


Review comment (suggestion):
The double quotes are necessary because you don't want the variable to expand to multiple words.

```
BOARD='foo bar'
"${BOARD}"  # becomes ["foo bar"] with Rust-like syntax
${BOARD}  # becomes ["foo", "bar"] with Rust-like syntax
```

Code after:
#!/bin/bash
# Copyright 2021 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#      http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# Creates a signature key and configures the public key.
# The device will not be locked down for testing purposes.
# Generates the binary and upgrades OpenSK.
# To be run from the OpenSK base path.

set -e

BOARD="$1"

./deploy.py --board="${BOARD}" --opensk --programmer=none $2
python3 -m tools.deploy_partition --board="${BOARD}" $2
if nrfjprog --reset --family NRF52 ; then
  echo "Upgrade finished!"
else
  echo "Please replug OpenSK to reboot"
fi
Repository: google/OpenSK
File: deploy.py (Python)

Code before:
#!/usr/bin/env py_virtual_env/bin/python3
# Copyright 2019-2023 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#      http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# Lint as: python3
# pylint: disable=C0111

from __future__ import absolute_import
from __future__ import division
from __future__ import print_function

import argparse
import collections
import copy
import os
import shutil

Review comment (suggestion):
No need for `env` now that we know the path.
```suggestion
#!py_virtual_env/bin/python3
```

Code after:
#!py_virtual_env/bin/python3
# Copyright 2019-2023 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#      http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# Lint as: python3
# pylint: disable=C0111

from __future__ import absolute_import
from __future__ import division
from __future__ import print_function

import argparse
import collections
import copy
import os
import shutil
Repository: google/OpenSK
File: tools/configure.py (Python)

Code before:
#!/usr/bin/env py_virtual_env/bin/python3
# Copyright 2020 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#      http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# Lint as: python3
"""Tools that implements vendor-specific CTAP2 commands to configure OpenSK."""

from __future__ import absolute_import
from __future__ import division
from __future__ import print_function

import argparse
import getpass
import datetime
import sys
from unittest.mock import patch

Review comment (suggestion):
Same here.
```suggestion
#!py_virtual_env/bin/python3
```

Code after:
#!py_virtual_env/bin/python3
# Copyright 2020 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#      http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# Lint as: python3
"""Tools that implements vendor-specific CTAP2 commands to configure OpenSK."""

from __future__ import absolute_import
from __future__ import division
from __future__ import print_function

import argparse
import getpass
import datetime
import sys
from unittest.mock import patch