Read and write BMP file in C

Question

I'm teaching myself a little C, and I've found this website. It has an exercise about manipulating a BMP file in C. I would like to know if I can make my code look more "idiomatic" or professional, from the format of the comments to the name of the variables.

I also have some other questions:

1. This is the standard way to handle errors in C? There is such a thing?
2. When programming in C, I hear a lot the term "robust". Is my program robust enough? For example, should I always check for null in a function that takes a pointer as a parameter? Should I check for errors every time I close a file?
3. In bmp.c, the _string_duplicate function returns a string (an error message) on the heap. But in case of an error (can't allocate enough memory) it returns a string literal. I'm not sure about which is the best way to handle the error here. This returned string is going to be freed by the caller, but in case of an error in _string_duplicate the caller will be freeing a pointer to a string literal, which is bad.

bmp.c

/*
 * A program to read, write, and crop BMP image files.
 * 
 */
#include <stdio.h>
#include <string.h>  // for strlen, strcopy
#include <stdlib.h>  // for malloc
#include "bmp.h"

// Correct values for the header
#define MAGIC_VALUE         0x4D42 
#define NUM_PLANE           1
#define COMPRESSION         0
#define NUM_COLORS          0
#define IMPORTANT_COLORS    0
#define BITS_PER_PIXEL      24
#define BITS_PER_BYTE       8

BMPImage *read_bmp(FILE *fp, char **error);
bool write_bmp(FILE *fp, BMPImage *image, char **error);
bool check_bmp_header(BMPHeader *bmp_header, FILE *fp);
void free_bmp(BMPImage *image);
long _get_file_size(FILE *fp);
int _get_image_size_bytes(BMPHeader *bmp_header);
int _get_image_row_size_bytes(BMPHeader *bmp_header);
int _get_bytes_per_pixel(BMPHeader  *bmp_header);
int _get_padding(BMPHeader *bmp_header);
int _get_position_x_row(int x, BMPHeader *bmp_header);
bool _check(bool condition, char **error, const char *error_message);
char *_string_duplicate(const char *string);

/*
 * Read a BMP image from an already open file.
 * 
 * - Postcondition: it is the caller's responsibility to free the memory
 *   for the error message and the returned image.
 * 
 * - Return: the image as a BMPImage on the heap.
 */
BMPImage *read_bmp(FILE *fp, char **error)
{    
    BMPImage *image = malloc(sizeof(*image));
    if (!_check(image != NULL, error, "Not enough memory"))
    {
        return NULL;
    }
    // Read header
    rewind(fp);
    int num_read = fread(&image->header, sizeof(image->header), 1, fp);
    if(!_check(num_read == 1, error, "Cannot read header"))
    {
        return NULL;
    }
    // Check header
    bool is_valid_header = check_bmp_header(&image->header, fp);
    if(!_check(is_valid_header, error, "Invalid BMP file"))
    {
        return NULL;
    }
    // Allocate memory for image data
    image->data = malloc(sizeof(*image->data) * image->header.image_size_bytes);
    if (!_check(image->data != NULL, error, "Not enough memory"))
    {
        return NULL;
    }
    // Read image data
    num_read = fread(image->data, image->header.image_size_bytes, 1, fp);
    if (!_check(num_read == 1, error, "Cannot read image"))
    {
        return NULL;
    }

    return image;
}

/*
 * Write an image to an already open file.
 * 
 * - Postcondition: it is the caller's responsibility to free the memory
 *   for the error message.
 * - Return: true if and only if the operation succeeded.
 */
bool write_bmp(FILE *fp, BMPImage *image, char **error)
{
    // Write header
    rewind(fp);
    int num_read = fwrite(&image->header, sizeof(image->header), 1, fp);
    if (!_check(num_read == 1, error, "Cannot write image"))
    {
        return false;
    }
    // Write image data
    num_read = fwrite(image->data, image->header.image_size_bytes, 1, fp);
    if (!_check(num_read == 1, error, "Cannot write image"))
    {
        return false;
    }

    return true;
}

/*
 * Test if the BMPHeader is consistent with itself and the already open image file.
 * 
 * Return: true if and only if the given BMPHeader is valid.
 */
bool check_bmp_header(BMPHeader* bmp_header, FILE* fp)
{
    /*
    A header is valid if:
    1. its magic number is 0x4d42,
    2. image data begins immediately after the header data (header->offset == BMP HEADER SIZE),
    3. the DIB header is the correct size (DIB_HEADER_SIZE),
    4. there is only one image plane,
    5. there is no compression (header->compression == 0),
    6. num_colors and  important_colors are both 0,
    7. the image has 24 bits per pixel,
    8. the size and image_size_bytes fields are correct in relation to the bits,
       width, and height fields and in relation to the file size.
    */
    return
        bmp_header->type == MAGIC_VALUE
        && bmp_header->offset == BMP_HEADER_SIZE
        && bmp_header->dib_header_size == DIB_HEADER_SIZE
        && bmp_header->num_planes == NUM_PLANE
        && bmp_header->compression == COMPRESSION
        && bmp_header->num_colors == NUM_COLORS && bmp_header->important_colors == IMPORTANT_COLORS
        && bmp_header->bits_per_pixel == BITS_PER_PIXEL
        && bmp_header->size == _get_file_size(fp) && bmp_header->image_size_bytes == _get_image_size_bytes(bmp_header);
}

/*
 * Free all memory referred to by the given BMPImage.
 */
void free_bmp(BMPImage *image)
{
    free(image->data);
    free(image);
}

/*
 * Create a new image containing the cropped portion of the given image.
 * 
 * - Params:
 *       x - the start index, from the left edge of the input image.
 *       y - the start index, from the top edge of the input image.
 *       w - the width of the new image.
 *       h - the height of the new image.
 * 
 * - Postcondition: it is the caller's responsibility to free the memory
 *   for the error message and the returned image.
 * 
 * - Return: the cropped image as a BMPImage on the heap.
 */
BMPImage *crop_bmp(BMPImage *image, int x, int y, int w, int h, char **error)
{
    BMPImage *new_image = malloc(sizeof(*new_image));
    // Check size of cropedd image is less or equal than the size of original image
    if (!_check
            (
                x + w <= image->header.width_px && y + h <= image->header.height_px,
                error,
                "The size of the new image should be equal or less than the size of the original")
            )
    {
        return NULL;
    }
    // Update new_image header
    new_image->header = image->header;
    new_image->header.width_px = w;
    new_image->header.height_px = h;
    new_image->header.image_size_bytes = _get_image_size_bytes(&new_image->header);
    new_image->header.size = BMP_HEADER_SIZE + new_image->header.image_size_bytes;
    // Allocate memory for image data
    new_image->data = malloc(sizeof(*new_image->data) * new_image->header.image_size_bytes);
    if(!_check(new_image->data != NULL, error, "Not enough memory"))
    {
        return NULL;
    }
    int position_y = y * _get_image_row_size_bytes(&image->header);
    int position_x_row = _get_position_x_row(x, &image->header);
    int new_index = 0;
    // Iterate image's columns
    for (int i = 0; i < h; i++)
    {
        // Iterate image's rows
        for (int j = 0; j < w; j++)
        {
            // Iterate image's pixels
            for(int k = 0; k < 3; k++)
            {              
                new_image->data[new_index] = image->data[position_y + position_x_row];
                new_index++;
                position_x_row++;
            }
        }
        // Add padding to new_image
        int padding = _get_padding(&new_image->header);
        for (int l = 0; l < padding; l++)
        {  
            new_image->data[new_index] = 0x00;
            new_index++;
        }
        position_y += _get_image_row_size_bytes(&image->header);
        position_x_row = _get_position_x_row(x, &image->header);
    }

    return new_image;
}

/* 
 * Return the size of the file.
 */
long _get_file_size(FILE *fp)
{   
    // Get current file position
    long current_position = ftell(fp);
    if (current_position == -1)
    {
        return -1;
    }
    // Set file position to the end
    if (fseek(fp, 0, SEEK_END) != 0)
    {
        return -2;
    }
    // Get current file position (now at the end)
    long file_size = ftell(fp);
    if (file_size == -1)
    {
        return -3;
    }
    // Restore previous file position
    if (fseek(fp, current_position, SEEK_SET) != 0)
    {
        return -4;
    }

    return file_size;
}

/* 
 * Return the size of the image in bytes.
 */
int _get_image_size_bytes(BMPHeader *bmp_header)
{
    return _get_image_row_size_bytes(bmp_header) * bmp_header->height_px;
}

/* 
 * Return the size of an image row in bytes.
 *  
 * - Precondition: the header must have the width of the image in pixels.
 */
int _get_image_row_size_bytes(BMPHeader *bmp_header)
{
    int bytes_per_row_without_padding = bmp_header->width_px * _get_bytes_per_pixel(bmp_header);
    return bytes_per_row_without_padding + _get_padding(bmp_header);
}

/*
 * Return size of padding in bytes.
 */ 
int _get_padding(BMPHeader *bmp_header)
{
    return (4 - (bmp_header->width_px * _get_bytes_per_pixel(bmp_header)) % 4) % 4;
}

/* 
 * Return the number of bytes per pixel.
 *  
 * Precondition:
 *     - the header must have the number of bits per pixel.
 */
int _get_bytes_per_pixel(BMPHeader  *bmp_header)
{
    return bmp_header->bits_per_pixel / BITS_PER_BYTE;
}

/*
 * Return the position of the pixel x from the beginning of a row.
 */ 
int _get_position_x_row(int x, BMPHeader *bmp_header)
{
    return x * _get_bytes_per_pixel(bmp_header);
}

/*
 * Check condition and set error message.
 */
bool _check(bool condition, char **error, const char *error_message)
{
    bool is_valid = true;
    if(!condition)
    {
        is_valid = false;
        if (*error == NULL)  // to avoid memory leaks
        {
            *error = _string_duplicate(error_message);
        }
    }    
    return is_valid;
}

/*
 * Make a copy of a string on the heap.
 * 
 * - Postcondition: the caller is responsible to free
 *   the memory for the string.
 */ 
char *_string_duplicate(const char *string)
{
    char *copy = malloc(sizeof(*copy) * (strlen(string) + 1));
    if (copy == NULL)
    {
        return "Not enough memory for error message";
    }
    strcpy(copy, string);

    return copy;
}

The header file:

bmp.h

#ifndef _BMP_H_  // prevent recursive inclusion
#define _BMP_H_

#include <stdint.h>
#include <stdbool.h>
#include <stdio.h>

#define BMP_HEADER_SIZE 54
#define DIB_HEADER_SIZE 40

#pragma pack(push)  // save the original data alignment
#pragma pack(1)     // Set data alignment to 1 byte boundary

// uint16_t is a 16-bit unsigned integer
// uint32_t is a 32-bit unsigned integer

typedef struct {
    uint16_t type;              // Magic identifier: 0x4d42
    uint32_t size;              // File size in bytes
    uint16_t reserved1;         // Not used
    uint16_t reserved2;         // Not used
    uint32_t offset;            // Offset to image data in bytes from beginning of file
    uint32_t dib_header_size;   // DIB Header size in bytes
    int32_t  width_px;          // Width of the image
    int32_t  height_px;         // Height of image
    uint16_t num_planes;        // Number of color planes
    uint16_t bits_per_pixel;    // Bits per pixel
    uint32_t compression;       // Compression type
    uint32_t image_size_bytes;  // Image size in bytes
    int32_t  x_resolution_ppm;  // Pixels per meter
    int32_t  y_resolution_ppm;  // Pixels per meter
    uint32_t num_colors;        // Number of colors
    uint32_t important_colors;  // Important colors
} BMPHeader;

#pragma pack(pop)  // restore the previous pack setting

typedef struct {
    BMPHeader header;
    unsigned char* data;
} BMPImage;

BMPImage* read_bmp(FILE* fp, char** error);
bool      check_bmp_header(BMPHeader* bmp_header, FILE* fp);
bool      write_bmp(FILE* fp, BMPImage* image, char** error);
void      free_bmp(BMPImage* image);
BMPImage* crop_bmp(BMPImage* image, int x, int y, int w, int h, char** error);

#endif  /* bmp.h */

And a simple test. I feel this one is a little messy. I'm not so sure about the helper functions I've made.

#include <stdio.h>
#include <stdlib.h>  // for EXIT_SUCCESS and EXIT_FAILURE
#include "bmp.h"

BMPImage *read_image(const char *filename, char **error);
void write_image(const char *filename, BMPImage *image, char **error);
FILE *_open_file(const char *filename, const char *mode);
void _handle_error(char **error, FILE *fp, BMPImage *image);
void _clean_up(FILE *fp, BMPImage *image, char **error);

int main(void)
{
    char *error = NULL;
    BMPImage *image = read_image("6x6_24bit.bmp", &error);
    write_image("copy.bmp", image, &error);
    BMPImage *crop_image = crop_bmp(image, 2, 3, 4, 2, &error);
    write_image("crop.bmp", crop_image, &error);

    bool is_valid = check_bmp_header(&crop_image->header, fopen("crop.bmp", "rb"));

    _clean_up(NULL, image, &error);
    _clean_up(NULL, crop_image, &error);

    return EXIT_SUCCESS;
}

BMPImage *read_image(const char *filename, char **error)
{
    FILE *input_ptr = _open_file(filename, "rb");

    BMPImage *image = read_bmp(input_ptr, error);
    if (*error != NULL)
    {
        _handle_error(error, input_ptr, image);
    }
    fclose(input_ptr);

    return image;
}

void write_image(const char *filename, BMPImage *image, char **error)
{
    FILE *output_ptr = _open_file(filename, "wb");

    if (!write_bmp(output_ptr, image, error))
    {
        _handle_error(error, output_ptr, image);
    }    
    fclose(output_ptr);
}

/*
 * Open file. In case of error, print message and exit.
 */
FILE *_open_file(const char *filename, const char *mode)
{
    FILE *fp = fopen(filename, mode);
    if (fp == NULL)
    {
        fprintf(stderr, "Could not open file %s", filename);

        exit(EXIT_FAILURE);
    } 

    return fp;
}

/*
 * Print error message and clean up resources.
 */
void _handle_error(char **error, FILE *fp, BMPImage *image)
{
    fprintf(stderr, "ERROR: %s\n", *error);
    _clean_up(fp, image, error);

    exit(EXIT_FAILURE);
}

/*
 * Close file and release memory.
 */
void _clean_up(FILE *fp, BMPImage *image, char **error)
{
    if (fp != NULL)
    {
        fclose(fp);
    }    
    free_bmp(image);
    free(*error);
}

Answer 1

I would like to know if I can make my code look more "idiomatic" or professional,

Packing

#pragma pack(push) #pragma pack(1) are not standard C. Yet BMPHeader is a pain due to the first member is 16-bit. Unless high portability is needed, stay with the non-standard packing for now.

unsigned char

Other struct members used fixed width types (good). Recommend to apply that to data. It is more informative and will force a necessary compiler error on a rare machine with 16-bit char.

typedef struct {
  BMPHeader header;
  // unsigned char* data;
  uint8_t* data;
} BMPImage;

bmp.h Comments

A user of the code may never see bmp.c, so comment important attributes in bmp.h. Consider more documentation in this file.

Take the below from bmp.c and put only in bmp.h - and so for all global functions. In the bmp.c leave a comment with the function if desired to "see bmp.h". You do not want to maintain dual documentation.

/*
 * Read a BMP image from an already open file.
 * 
 * - Postcondition: it is the caller's responsibility to free the memory
 *   for the error message and the returned image.
 * 
 * - Return: the image as a BMPImage on the heap.
 */

What does true mean in bool check_bmp_header()?

uint16_t is a 16-bit unsigned integer is an unnecessary comment.

Magic identifier: 0x4d42 is more informative as 'B' 'M'

Its is not stated here that *error in read_bmp(FILE* fp, char** error) requires prior assignment before calling this function.

Unclear why int x4 in crop_bmp(BMPImage* image, int x, int y, int w, int h, char** error) when the format type uses int32_t. I'd recommend the same type. (or at least consider what code needs to do when int is narrower than int32_t.)

Good use of minimal #include <> files.

Error message

The error handling, often a tricky point, lacks symmetry here amongst the various ..._bmp().

As error messages are copy of string literals, instead of copying the text, copy the pointer and make know the user need not free it.

Consider instead of a sometimes char** error function parameter, add a .error member to BMPImage.

Naming

The bmp.h file uses BMP_..., DIB_..., BMP..., ..._bmp naming. I would strive for consistency and use only bmp_... and BMP_...

bmp.c: Use static functions

_get_file_size(), ... _string_duplicate(), should bestatic. these function names are not needed outside `bmp.c

bmp.c: global functions

No need for another declaration of functions found in bmp.h

include test

In bmp.c only, code #include "bmp.h" first to test that it compiles without prior #includes.

#include "bmp.h"
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
// #include "bmp.h"
#include "other_user_header_files1.h"
#include "other_user_header_files2.h"

Comments like // for strlen, strcopy are a pain to maintain. I recommend dropping that for standard headers.

No computational check

sizeof(*image->data) * image->header.image_size_bytes lacks overflow protection. Be very careful with external data from files that can cause undefined behavior with mis-szied allocation. Watch out for .image_size_bytes == 0 as the allocation may be NULL (which is not a "Not enough memory", but other concern).

uint64_t sz = image->header.image_size_bytes
sz *= sizeof(*image->data);
image->data = NULL;
if (sz <= SIZE_MAX) {
  image->data = malloc((size_t) sz);
}
if (image->data == NULL && sz > 0) {
  // assign error
}

free() style

free() tolerates free(NULL) and so I recommend to do so here with free_bmp(BMPImage *image)

void free_bmp(BMPImage *image) {
    if (image) {
      free(image->data);
      free(image);
    } 
}

---

Advanced:

Endian

.bmp file format assume an endian (little) and so each member of BMPHeader requires an endian adjustment should code compile on a non-little endian machine for both reading and writing.

---

Minor:

Allocating

Good use of allocating by size of de-referenced pointer rather than type. Style thought: () not needed

image = malloc(sizeof(*image));
// or
image = malloc(sizeof *image);

---

GTG

Answer 2

And one more thing:
You define the return value as char*, then you proceed to return "copy" with malloc, but if it fails, you return a const char*, which might be free'd.

**** Error in `/root/CLionProjects/BitmapTest/cmake-build-debug/BitmapTest':
munmap_chunk(): invalid pointer: 0x0000000000401d7b ***
======= Backtrace: =========
[...]

You should malloc this string.

char *_string_duplicate(const char *string)
{
    char *copy = (char*) malloc(sizeof(*copy) * (strlen(string) + 1));
    if (copy == NULL)
    {
        // return "Not enough memory for error message";
        const char* error_message = "Not enough memory for error message";
        size_t len = strlen(error_message);
        char* error = (char*) malloc(len*sizeof(char) + 1);
        if(error == NULL)
           { /* TODO: return null or exit the program */ }
        strcpy(error, error_message);
        return error;
    }

    strcpy(copy, string);
    return copy;
}