ronantakizawa/codereview-bench huggingface dataset:
Repository: google/OpenSK
File: deploy.sh (Shell)

Code before:
  cargo build \
    --release \
    --target=thumbv7em-none-eabi \
    --features="${feature_list}"

  mkdir -p "target/tab"
  cp "target/thumbv7em-none-eabi/release/ctap2" "$elf_file_name"

  elf2tab \
    "${elf2tab_package_param}" "ctap2" \
    -o "${tab_folder}/ctap2.tab" \
    "$elf_file_name" \
    --stack 16384 \
    --app-heap 90000 \
    --kernel-heap 1024 \
    --protected-region-size=64
}

build_crypto_bench () {
  # Flatten the array
  # This is equivalent to the following python snippet: ' '.join(arr).replace(' ', ',')
  local feature_list=$(IFS=$'\n'; echo "$@")
  if [ "X${feature_list}" != "X" ]
  then
    feature_list="${feature_list// /,}"
  fi

  cargo build \
    --release \
    --target=thumbv7em-none-eabi \
    --features="${feature_list}" \
    --example crypto_bench

  mkdir -p "target/tab"
  cp "target/thumbv7em-none-eabi/release/examples/crypto_bench" "$elf_file_name"

  elf2tab \
    "${elf2tab_package_param}" "crypto_bench" \
    -o "${tab_folder}/crypto_bench.tab" \
    "$elf_file_name" \
    --stack 16384 \
    --app-heap 90000 \
    --kernel-heap 1024 \
    --protected-region-size=64
}

deploy_tock () {
  if [ "x$board" = "x" ];
  then
    echo "$error_text You must set the board in order to install Tock OS (example: \`board=nrf52840dk $0 os\`)"
    display_supported_boards

Review comment (refactor):
I believe this code should not be duplicated.

```sh
comma_separated () {
  local feature_list=$(IFS=$'\n'; echo "$@")
￼  if [ "X${feature_list}" != "X" ]
￼  then
￼    echo "${feature_list// /,}"
￼  fi
}
```

We would then have `local feature_list="$(comma_separated "$@")"` in both `build_app` and here.

Code after:
}

build_app () {
  local feature_list="$(comma_separated "$@")"
  cargo build \
    --release \
    --target=thumbv7em-none-eabi \
    --features="${feature_list}"

  mkdir -p "target/tab"
  cp "target/thumbv7em-none-eabi/release/ctap2" "$elf_file_name"

  elf2tab \
    "${elf2tab_package_param}" "ctap2" \
    -o "${tab_folder}/ctap2.tab" \
    "$elf_file_name" \
    --stack 16384 \
    --app-heap 90000 \
    --kernel-heap 1024 \
    --protected-region-size=64
}

build_crypto_bench () {
  local feature_list="$(comma_separated "$@")"
  cargo build \
    --release \
    --target=thumbv7em-none-eabi \
    --features="${feature_list}" \
    --example crypto_bench

  mkdir -p "target/tab"
  cp "target/thumbv7em-none-eabi/release/examples/crypto_bench" "$elf_file_name"

  elf2tab \
    "${elf2tab_package_param}" "crypto_bench" \
    -o "${tab_folder}/crypto_bench.tab" \
    "$elf_file_name" \
    --stack 16384 \
    --app-heap 90000 \
    --kernel-heap 1024 \
    --protected-region-size=64
}

deploy_tock () {
  if [ "x$board" = "x" ];
  then
    echo "$error_text You must set the board in order to install Tock OS (example: \`board=nrf52840dk $0 os\`)"
    display_supported_boards
    return 1
  fi
  if [ "${supported_boards[$board]+x}" = "x" -a -d "third_party/tock/boards/nordic/$board" ]
Repository: google/OpenSK
File: deploy.sh (Shell)

Code before:
    # Version: 2
    echo -n "0200"
    # Header size: 0x10
    echo -n "1000"
    # Total size: 0x10000
    echo -n "00000100"
    # Flags: 0
    echo -n "00000000"
    # Checksum
    echo -n "02001100"
  ) | xxd -p -r > "${tab_folder}/padding.bin"
}

comma_separated () {
  # Flatten the array
  # This is equivalent to the following python snippet: ' '.join(arr).replace(' ', ',')
  local list=$(IFS=$'\n'; echo "$@")
  if [ "X${list}" != "X" ]
  then
    feature_list="${list// /,}"
  fi
  echo ${list}
}

build_app () {
  local feature_list="$(comma_separated $@)"
  cargo build \
    --release \
    --target=thumbv7em-none-eabi \
    --features="${feature_list}"

  mkdir -p "target/tab"
  cp "target/thumbv7em-none-eabi/release/ctap2" "$elf_file_name"

  elf2tab \
    "${elf2tab_package_param}" "ctap2" \
    -o "${tab_folder}/ctap2.tab" \
    "$elf_file_name" \
    --stack 16384 \
    --app-heap 90000 \
    --kernel-heap 1024 \
    --protected-region-size=64
}

build_crypto_bench () {
  local feature_list="$(comma_separated $@)"
  cargo build \
    --release \
    --target=thumbv7em-none-eabi \
    --features="${feature_list}" \
    --example crypto_bench

Review comment (suggestion):
This is usually a bad pattern (`"$@"` should be preferred to `$@`), but in that case we don't expect spaces in feature names. Also the `comma_separated` function does not handle it well neither. And this is hopefully temporary until this becomes a python script.

Code after:
    # Version: 2
    echo -n "0200"
    # Header size: 0x10
    echo -n "1000"
    # Total size: 0x10000
    echo -n "00000100"
    # Flags: 0
    echo -n "00000000"
    # Checksum
    echo -n "02001100"
  ) | xxd -p -r > "${tab_folder}/padding.bin"
}

comma_separated () {
  # Flatten the array
  # This is equivalent to the following python snippet: ' '.join(arr).replace(' ', ',')
  local list=$(IFS=$'\n'; echo "$@")
  if [ "X${list}" != "X" ]
  then
    feature_list="${list// /,}"
  fi
  echo ${list}
}

build_app () {
  local feature_list="$(comma_separated "$@")"
  cargo build \
    --release \
    --target=thumbv7em-none-eabi \
    --features="${feature_list}"

  mkdir -p "target/tab"
  cp "target/thumbv7em-none-eabi/release/ctap2" "$elf_file_name"

  elf2tab \
    "${elf2tab_package_param}" "ctap2" \
    -o "${tab_folder}/ctap2.tab" \
    "$elf_file_name" \
    --stack 16384 \
    --app-heap 90000 \
    --kernel-heap 1024 \
    --protected-region-size=64
}

build_crypto_bench () {
  local feature_list="$(comma_separated "$@")"
  cargo build \
    --release \
    --target=thumbv7em-none-eabi \
    --features="${feature_list}" \
    --example crypto_bench
Repository: google/OpenSK
File: src/embedded_flash/buffer.rs (Rust)

Code before:
// Copyright 2019 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.

use super::{Index, Storage, StorageError, StorageResult};
#[cfg(not(feature = "std"))]
use alloc::boxed::Box;
#[cfg(not(feature = "std"))]
use core as std;

pub struct BufferStorage {
    storage: Box<[u8]>,
    options: BufferOptions,
    word_writes: Box<[usize]>,
    page_erases: Box<[usize]>,
    snapshot: Snapshot,
}

#[derive(Copy, Clone, Debug)]
pub struct BufferOptions {
    /// Size of a word in bytes.
    pub word_size: usize,

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

    /// How many times a word can be written between page erasures
    pub max_word_writes: usize,

    /// How many times a page can be erased.
    pub max_page_erases: usize,

    /// Bits cannot be written from 0 to 1.
    pub strict_write: bool,

Review comment (suggestion):
I find this confusing. The code should rather use `core`, and making sure that `core` is recognized in a std context for the tests.

Code after:
// Copyright 2019 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.

use super::{Index, Storage, StorageError, StorageResult};
use alloc::boxed::Box;

pub struct BufferStorage {
    storage: Box<[u8]>,
    options: BufferOptions,
    word_writes: Box<[usize]>,
    page_erases: Box<[usize]>,
    snapshot: Snapshot,
}

#[derive(Copy, Clone, Debug)]
pub struct BufferOptions {
    /// Size of a word in bytes.
    pub word_size: usize,

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

    /// How many times a word can be written between page erasures
    pub max_word_writes: usize,

    /// How many times a page can be erased.
    pub max_page_erases: usize,

    /// Bits cannot be written from 0 to 1.
    pub strict_write: bool,
}

impl BufferStorage {
Repository: google/OpenSK
File: run_desktop_tests.sh (Shell)

Code before:
  cd ../..
  cd libraries/crypto
  RUSTFLAGS='-C target-feature=+aes' cargo test --release --features std,derive_debug
  cd ../..
  cargo test --release --features std

  echo "Running unit tests on the desktop (debug mode)..."
  cd libraries/cbor
  cargo test --features std
  cd ../..
  cd libraries/crypto
  RUSTFLAGS='-C target-feature=+aes' cargo test --features std,derive_debug
  cd ../..
  cargo test --features std

  echo "Running unit tests on the desktop (release mode + CTAP1)..."
  cargo test --release --features std,with_ctap1

  echo "Running unit tests on the desktop (debug mode + CTAP1)..."
  cargo test --features std,with_ctap1

  echo "Running unit tests on the desktop (release mode + CTAP2.1)..."
  cargo test --release --features std,with_ctap2_1

  echo "Running unit tests on the desktop (debug mode + CTAP2.1)..."
  cargo test --features std,with_ctap2_1
fi


Review comment (suggestion):
So we never test `with_ctap1` + `with_ctap2_1`? Having both is a relevant use case, and may cause issues like exceeding available memory, so I'd test that too.

Code after:
  cd ../..
  cd libraries/crypto
  RUSTFLAGS='-C target-feature=+aes' cargo test --release --features std,derive_debug
  cd ../..
  cargo test --release --features std

  echo "Running unit tests on the desktop (debug mode)..."
  cd libraries/cbor
  cargo test --features std
  cd ../..
  cd libraries/crypto
  RUSTFLAGS='-C target-feature=+aes' cargo test --features std,derive_debug
  cd ../..
  cargo test --features std

  echo "Running unit tests on the desktop (release mode + CTAP1)..."
  cargo test --release --features std,with_ctap1

  echo "Running unit tests on the desktop (debug mode + CTAP1)..."
  cargo test --features std,with_ctap1

  echo "Running unit tests on the desktop (release mode + CTAP2.1)..."
  cargo test --release --features std,with_ctap2_1

  echo "Running unit tests on the desktop (debug mode + CTAP2.1)..."
  cargo test --features std,with_ctap2_1

  echo "Running unit tests on the desktop (release mode + CTAP1 + CTAP2.1)..."
  cargo test --release --features std,with_ctap1,with_ctap2_1

  echo "Running unit tests on the desktop (debug mode + CTAP1 + CTAP2.1)..."
  cargo test --features std,with_ctap1,with_ctap2_1
fi
Repository: google/OpenSK
File: libraries/cbor/src/macros.rs (Rust)

Code before:
}

#[macro_export]
macro_rules! cbor_map_options {
    // trailing comma case
    ( $( $key:expr => $value:expr, )+ ) => {
        cbor_map_options! ( $($key => $value),+ )
    };

    ( $( $key:expr => $value:expr ),* ) => {
        {
            let mut _map = ::alloc::collections::BTreeMap::<_, $crate::values::Value>::new();
            extend_cbor_map_options! ( &mut _map, $( $key => $value, )* );
            $crate::values::Value::Map(_map)
        }
    };
}

#[macro_export]
macro_rules! extend_cbor_map_options {
    // Add trailing comma if missing.
    ( &mut $initial:expr, $( $key:expr => $value:expr ),+ ) => {
        extend_cbor_map_options! ( &mut $initial, $($key => $value,)+ )
    };

    ( &mut $initial:expr, $( $key:expr => $value:expr, )* ) => {
        {
            // The import is unused if the list is empty.
            #[allow(unused_imports)]
            use $crate::values::{IntoCborKey, IntoCborValueOption};
            $(
                if let Some(val) = $value.into_cbor_value_option() {
                    $initial.insert($key.into_cbor_key(), val);
                }
            )*
        }
    };
}

#[macro_export]
macro_rules! cbor_map_btree {
    ( $tree:expr ) => {
        $crate::values::Value::Map($tree)
    };
}

#[macro_export]
macro_rules! cbor_array {
    // trailing comma case
    ( $( $value:expr, )+ ) => {
        cbor_array! ( $($value),+ )

Review comment (suggestion):
This should be named `$map` instead of `$initial`. This only contains the initial map at the beginning, then it's mutated.

Code after:
}

#[macro_export]
macro_rules! cbor_map_options {
    // trailing comma case
    ( $( $key:expr => $value:expr, )+ ) => {
        cbor_map_options! ( $($key => $value),+ )
    };

    ( $( $key:expr => $value:expr ),* ) => {
        {
            // The import is unused if the list is empty.
            #[allow(unused_imports)]
            use $crate::values::{IntoCborKey, IntoCborValueOption};
            let mut _map = ::alloc::collections::BTreeMap::<_, $crate::values::Value>::new();
            $(
            {
                let opt: Option<$crate::values::Value> = $value.into_cbor_value_option();
                if let Some(val) = opt {
                    _map.insert($key.into_cbor_key(), val);
                }
            }
            )*
            $crate::values::Value::Map(_map)
        }
    };
}

#[macro_export]
macro_rules! cbor_map_btree {
    ( $tree:expr ) => {
        $crate::values::Value::Map($tree)
    };
}

#[macro_export]
macro_rules! cbor_array {
    // trailing comma case
    ( $( $value:expr, )+ ) => {
        cbor_array! ( $($value),+ )
    };

    ( $( $value:expr ),* ) => {
        {
            // The import is unused if the list is empty.
            #[allow(unused_imports)]
            use $crate::values::IntoCborValue;
            $crate::values::Value::Array(vec![ $( $value.into_cbor_value(), )* ])
        }
    };
}
Repository: google/OpenSK
File: libraries/cbor/src/macros.rs (Rust)

Code before:
            )*
            $crate::values::Value::Map(_map)
        }
    };
}

#[macro_export]
macro_rules! cbor_map_options {
    // trailing comma case
    ( $( $key:expr => $value:expr, )+ ) => {
        cbor_map_options! ( $($key => $value),+ )
    };

    ( $( $key:expr => $value:expr ),* ) => {
        {
            let mut _map = ::alloc::collections::BTreeMap::<_, $crate::values::Value>::new();
            extend_cbor_map_options! ( &mut _map, $( $key => $value, )* );
            $crate::values::Value::Map(_map)
        }
    };
}

#[macro_export]
macro_rules! extend_cbor_map_options {
    // Add trailing comma if missing.
    ( &mut $map:expr, $( $key:expr => $value:expr ),+ ) => {
        extend_cbor_map_options! ( &mut $map, $($key => $value,)+ )
    };

    ( &mut $map:expr, $( $key:expr => $value:expr, )* ) => {
        {
            // The import is unused if the list is empty.
            #[allow(unused_imports)]
            use $crate::values::{IntoCborKey, IntoCborValueOption};
            $(
                if let Some(val) = $value.into_cbor_value_option() {
                    $map.insert($key.into_cbor_key(), val);
                }
            )*
        }
    };
}

#[macro_export]
macro_rules! cbor_map_btree {
    ( $tree:expr ) => {
        $crate::values::Value::Map($tree)
    };
}

#[macro_export]

Review comment (suggestion):
The syntax shouldn't require a `&mut` token. The caller could obtain a mutable reference without directly using `&mut`, for example by calling a function which returns a `&mut T`.

Code after:
            )*
            $crate::values::Value::Map(_map)
        }
    };
}

#[macro_export]
macro_rules! cbor_map_options {
    // trailing comma case
    ( $( $key:expr => $value:expr, )+ ) => {
        cbor_map_options! ( $($key => $value),+ )
    };

    ( $( $key:expr => $value:expr ),* ) => {
        {
            // The import is unused if the list is empty.
            #[allow(unused_imports)]
            use $crate::values::{IntoCborKey, IntoCborValueOption};
            let mut _map = ::alloc::collections::BTreeMap::<_, $crate::values::Value>::new();
            $(
            {
                let opt: Option<$crate::values::Value> = $value.into_cbor_value_option();
                if let Some(val) = opt {
                    _map.insert($key.into_cbor_key(), val);
                }
            }
            )*
            $crate::values::Value::Map(_map)
        }
    };
}

#[macro_export]
macro_rules! cbor_map_btree {
    ( $tree:expr ) => {
        $crate::values::Value::Map($tree)
    };
}

#[macro_export]
macro_rules! cbor_array {
    // trailing comma case
    ( $( $value:expr, )+ ) => {
        cbor_array! ( $($value),+ )
    };

    ( $( $value:expr ),* ) => {
        {
            // The import is unused if the list is empty.
            #[allow(unused_imports)]
            use $crate::values::IntoCborValue;
Repository: google/OpenSK
File: libraries/cbor/fuzz/fuzz_targets/fuzz_target_cbor.rs (Rust)

Code before:
#![no_main]
#[macro_use]
extern crate libfuzzer_sys;
extern crate alloc;
extern crate cbor;

use alloc::vec::Vec;

fuzz_target!(|data: &[u8]| {
    let encoded = cbor::read(data);
    if let Ok(value) = encoded {
	let mut decoded = Vec::new();
	let result = cbor::write(value, &mut decoded);
	assert!(result);
	assert_eq!(decoded, data);
    };
});



Review comment (suggestion):
If you want, this whole function could be written:

```rust
if let Ok(value) = cbor::read(data) {
    let mut result = Vec::new();
    assert!(cbor::write(value, &mut result));
    assert_eq!(result, data);
}
```

Code after:
#![no_main]
#[macro_use]
extern crate libfuzzer_sys;
extern crate alloc;
extern crate cbor;

use alloc::vec::Vec;

fuzz_target!(|data: &[u8]| {
    if let Ok(value) = cbor::read(data) {
        let mut result = Vec::new();
        assert!(cbor::write(value, &mut result));
        assert_eq!(result, data);
    };
});
Repository: google/OpenSK
File: src/lib.rs (Rust)

Code before:
// Copyright 2019 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.

#![cfg_attr(not(feature = "std"), no_std)]

#[macro_use]
extern crate alloc;
#[cfg(not(feature = "fuzzing"))]
extern crate lang_items;
extern crate libtock_core;
extern crate libtock_drivers;
#[macro_use]
extern crate cbor;
#[macro_use]
extern crate arrayref;

#[cfg(feature = "fuzzing")]
pub mod ctap;
pub mod embedded_flash;


Review comment (suggestion):
When you run 

> ./deploy.py --opensk --board=nrf52840dk

you get

> error: unused `#[macro_use]` import

(You also see those with `./run_desktop_tests.sh`.)

Code after:
// Copyright 2019 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.

#![cfg_attr(not(feature = "std"), no_std)]

#[macro_use]
extern crate alloc;
#[macro_use]
extern crate arrayref;
#[macro_use]
extern crate cbor;
extern crate lang_items;
extern crate libtock_core;
extern crate libtock_drivers;

pub mod ctap;
pub mod embedded_flash;
Repository: google/OpenSK
File: fuzz/fuzz_targets/fuzz_target_split_assemble.rs (Rust)

Code before:
#![no_main]

extern crate ctap2;
extern crate libtock_drivers;
#[macro_use]
extern crate arrayref;

use ctap2::ctap::hid::receive::MessageAssembler;
use ctap2::ctap::hid::send::HidPacketIterator;
use ctap2::ctap::hid::{HidPacket, Message};
use libfuzzer_sys::fuzz_target;
use libtock_drivers::timer::Timestamp;

const DUMMY_TIMESTAMP: Timestamp<isize> = Timestamp::from_ms(0);
const PACKET_TYPE_MASK: u8 = 0x80;

// Converts a byte slice into Message
fn raw_to_message(data: &[u8], len: usize) -> Message {
    if len <= 4 {
        let mut cid = [0; 4];
        cid[..len].copy_from_slice(data);
        Message {
            cid,
            cmd: 0,
            payload: vec![],
        }
    } else {
        Message {
            cid: array_ref!(data, 0, 4).clone(),
            cmd: data[4],
            payload: data[5..].to_vec(),
        }
    }
}

/* Fuzzing HID packets splitting and assembling functions*/
fuzz_target!(|data: &[u8]| {
    let Message {
        cid,
        mut cmd,
        payload,
    } = raw_to_message(data, data.len());
    if let Some(hid_packet_iterator) = HidPacketIterator::new(Message {

Review comment (suggestion):
The function seems to have redundant information. In Rust, a reference to a slice is a "fat pointer", i.e. it contains both a pointer to the slice and the length of the slice. So here, the `len` argument is redundant with `data`. You can replace all usages of `len` with `data.len()` in this function and remove the argument.

Code after:
#![no_main]

extern crate ctap2;
extern crate libtock_drivers;
#[macro_use]
extern crate arrayref;

use ctap2::ctap::hid::receive::MessageAssembler;
use ctap2::ctap::hid::send::HidPacketIterator;
use ctap2::ctap::hid::{HidPacket, Message};
use libfuzzer_sys::fuzz_target;
use libtock_drivers::timer::Timestamp;

const DUMMY_TIMESTAMP: Timestamp<isize> = Timestamp::from_ms(0);
const PACKET_TYPE_MASK: u8 = 0x80;

// Converts a byte slice into Message
fn raw_to_message(data: &[u8]) -> Message {
    if data.len() <= 4 {
        let mut cid = [0; 4];
        cid[..data.len()].copy_from_slice(data);
        Message {
            cid,
            cmd: 0,
            payload: vec![],
        }
    } else {
        Message {
            cid: array_ref!(data, 0, 4).clone(),
            cmd: data[4],
            payload: data[5..].to_vec(),
        }
    }
}

/* Fuzzing HID packets splitting and assembling functions*/
fuzz_target!(|data: &[u8]| {
    let mut message = raw_to_message(data);
    if let Some(hid_packet_iterator) = HidPacketIterator::new(message.clone()) {
        let mut assembler = MessageAssembler::new();
        let packets: Vec<HidPacket> = hid_packet_iterator.collect();
        if let Some((last_packet, first_packets)) = packets.split_last() {
            for packet in first_packets {