Array breaking in Pebble C

Question

I'm trying to create a simple dice-rolling application in Pebble using C on CloudPebble. I have an array of different die sizes you can scroll through using Up/Down, and you roll (currently just generate a random number, it'll get fancier later) using the middle button. There's also a label at the top that displays the current die.

It's mostly working, but there's a bug I can't figure out for the life of me:

• When scrolling through the dice, at least one of the dice will not display the die number
• The broken die will still roll, but the rand() function doesn't seem to have a cap (or has a cap of maybe 1000; the highest I've rolled on a broken die to test was 880)

When it first started, the D6 was broken. I tinkered and tweaked and added some code to see what was going on in the background, but suddenly the D6 started working (no idea why) and the D20 was broken. I tried changing the "20" item in the array to "35" just to see what it would do, and the now-D35 worked, but the D2 and D4 were broken. I changed it back, and different dice broke.

Attached is my current code, which is currently showing D20 and D4 as broken. I've stripped out all the junk and reworking and side code I was using for troubleshooting, since none of it helped.

Can anyone please help explain why the dice are breaking seemingly at random, and how to fix it? Thank you!

#include <pebble.h>

static Window *s_main_window;
static TextLayer *s_label_layer;
static TextLayer *s_output_layer;

static int dice_select = 6;
static char *die_label = "D";
static char *dice_array[] = {"2", "4", "6", "8", "10", "12", "20", "100", NULL};

// === Separate Processes ===

    static void update_label(char *die) {
      die_label[1] = '\0';
      strcat(die_label, die);
      text_layer_set_text(s_label_layer, die_label);
    }

// === Main Window Processes ===

static void main_window_load(Window *window) {
  Layer *window_layer = window_get_root_layer(window);
  GRect window_bounds = layer_get_bounds(window_layer);

  // Create label TextLayer
  s_label_layer = text_layer_create(GRect(5, 0, window_bounds.size.w - 10, 24));
  text_layer_set_font(s_label_layer, fonts_get_system_font(FONT_KEY_GOTHIC_18_BOLD));
  text_layer_set_text_alignment(s_label_layer, GTextAlignmentCenter);
  text_layer_set_text(s_label_layer, "D20");
  text_layer_set_overflow_mode(s_label_layer, GTextOverflowModeWordWrap);
  layer_add_child(window_layer, text_layer_get_layer(s_label_layer));

  // Create output TextLayer
  s_output_layer = text_layer_create(GRect(5, 24, window_bounds.size.w - 10, window_bounds.size.h - 24));
  text_layer_set_font(s_output_layer, fonts_get_system_font(FONT_KEY_GOTHIC_28));
  text_layer_set_text(s_output_layer, "No button pressed yet.");
  text_layer_set_overflow_mode(s_output_layer, GTextOverflowModeWordWrap);
  layer_add_child(window_layer, text_layer_get_layer(s_output_layer));
}

static void main_window_unload(Window *window) {
  // Destroy output TextLayer
  text_layer_destroy(s_label_layer);
  text_layer_destroy(s_output_layer);
}


// === Button Handlers ===

static void up_click_handler(ClickRecognizerRef recognizer, void *context) {
  // Increment dice selection
  if(dice_select < 7) {
    dice_select += 1;
  } 
  update_label(dice_array[dice_select]);
}

static void select_click_handler(ClickRecognizerRef recognizer, void *context) {
  // Establish die selection
  char *die = dice_array[dice_select];     
  int die_sides = atoi(die);

  // Roll die
  int result = rand() % die_sides + 1;
  char *result_str = "";
  snprintf(result_str, sizeof(result_str), "%d", result);

  // Print result
  text_layer_set_text(s_output_layer, result_str);
}

static void down_click_handler(ClickRecognizerRef recognizer, void *context) {
  // Increment dice selection
  if(dice_select > 0) {
    dice_select -= 1;
  }
  update_label(dice_array[dice_select]);
}

static void click_config_provider(void *context) {
  // Register the ClickHandlers
  window_single_click_subscribe(BUTTON_ID_UP, up_click_handler);
  window_single_click_subscribe(BUTTON_ID_SELECT, select_click_handler);
  window_single_click_subscribe(BUTTON_ID_DOWN, down_click_handler);
}



// === Initialize/Deinitialize App ===

static void init() {
  // Create main Window
  s_main_window = window_create();
  window_set_window_handlers(s_main_window, (WindowHandlers) {
    .load = main_window_load,
    .unload = main_window_unload,
  });
  window_set_click_config_provider(s_main_window, click_config_provider);
  window_stack_push(s_main_window, true);
}

static void deinit() {
  // Destroy main Window
  window_destroy(s_main_window);
}

int main(void) {
  init();
  app_event_loop();
  deinit();
}

(Note: I'm an untrained self-taught beginner, so the answer to all questions like "Why did you do this that way?" is "Because I didn't know any better.")

EDIT: New code as added per comments.

This part works great:

static char die_label[32] = "D";

static void update_label(char *die) {
  snprintf(die_label, sizeof(die_label), "D%s", die);
  text_layer_set_text(s_label_layer, die_label);
}

But this part will no longer print the roll result:

  // Roll die
  int result = rand() % die_sides + 1;
  char result_str[32];
  snprintf(result_str, sizeof(result_str), "%d", result);

  // Print result
  text_layer_set_text(s_output_layer, result_str);

Answer 1

The problem is this line

static char *die_label = "D";

That points die_label to a region of memory that a) should not be written to, and b) only has space for two characters, the D and the \0 terminator. So the strcat is writing into memory that it shouldn't be.

The solution is to declare die_label as

static char die_label[32] = "D";   // reserve space for 32 characters

and then I suggest changing the update function to

static void update_label(char *die) 
{
  sprintf(die_label, "D%s", die);
  text_layer_set_text(s_label_layer, die_label);
}

---

You have the same problem with these two lines

char *result_str = "";
snprintf(result_str, sizeof(result_str), "%d", result);

In this instance, you only have space for one character, but the length that is passed to snprintf is the sizeof a pointer, which is either 4 or 8 bytes. I would change those lines to

char result_str[32];
sprintf( result_str, "%d", result );

---

In response to the updated question:

Yes, I wondered about that. Evidently text_layer_set_text doesn't make a copy of the string, it just keeps a pointer to it. That means that the result_str cannot be a local variable, since a local variable will go out of scope as soon as the function returns. The solution is to declare result_str at the top of the file just like die_label, or declare it as static in the function itself.