Concatenate file (single byte xor)

[Polyphony]

So this is actually a pretty old project of mine, I wrote it on a whim and liked the way it turned out.  I'm a big fan of chaining simple programs together (or I'm using that as my excuse  ) to accomplish a more complex task.  I was also inspired to post this from ca0s' crypter post which as far as I know, also impliments a simple xor 'encryption'.  Anyways, here's the code circa early April 2015.

Code: (C) [Select]

#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>

#define KEY 0x2e
#define xor(x) ((((x) >= 0) && ((x) <= 255)) ? ((x) ^= KEY) : ((x) = 0))
#define chkret(func, var) \
        if (!(var)) { \
            fprintf(stderr, func " has failed.\n"); \
            return -1; \
        }

unsigned char *bptr;

size_t fsize(const char *fn);
int bpull(FILE *fp, unsigned char *b, const char *fn, size_t s);

int main(int argc, char *argv[]) {
    char *fn;
    FILE *fp;
    int r;
    size_t s, i;

    if (argc != 2) {
        fprintf(stderr, "Usage: ./xcat <file>\n");
        return -1;
    }

    /* the chkret(func, var) macro checks if the return value of each
     * function is zero.  If it isn't, then we return(-1);  The idea was
     * to cut down on some of the "if" statement clutter that xcat was
     * suffering from checking return values manually. */
    fn = argv[1];
    s = fsize(fn);
    chkret("fsize", s);

    fp = fopen(fn, "rb");
    chkret("fopen", fp);

    bptr = malloc(sizeof(unsigned char) * s);
    chkret("malloc", bptr);

    r = bpull(fp, bptr, fn, s);
    chkret("bpull", r);

    for(i = 0; i < s; ++i)
        xor(bptr[i]);

    /* make this a user specified fp in the future? I like this
     * functionality */
    fwrite(bptr, 1, s, stdout);

    fclose(fp);
    free(bptr);
    bptr = NULL;

    return 0;
}

size_t fsize(const char *fn) {
    struct stat st;
    return (stat(fn, &st) == 0) ? st.st_size : 0;
}

int bpull(FILE *fp, unsigned char *b, const char *fn, size_t s) {
    return ((s-1) == fread(b, sizeof(unsigned char), s, fp)) ? 0 : -1;
}


[HTH]

You never did make it into the project I reccomended but +1 either way

[Polyphony]

Yeah, I kind of feel bad that it didn't get it to the next level, but maybe I'll give it another shot. I hadn't posted anything in a long time so I figured I would post some new code of mine.

[ArkPhaze]

'xor' is an operator keyword. I would not recommend creating a #define using the same name.

Code: [Select]

#define xor(x) ((((x) >= 0) && ((x) <= 255)) ? ((x) ^= KEY) : ((x) = 0))
(x) >= 0 will always return true too since you're using an unsigned type.

sizeof(char) will always be 1 too.

Code: [Select]

bptr = malloc(sizeof(unsigned char) * s);
You only need to malloc(s).

You have an unused parameter too:

Code: [Select]

const char *fn
Lastly, your chkret macro is convenient, but it does not guarantee memory is freed properly as it should in the case that files and memory are successfully closed and freed. return -1; happens before any of this is possible:

Code: [Select]

fclose(fp);
  free(bptr);
In the case that these should be called.

Maybe something like this would be better:

Code: [Select]

#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>

#define KEY 0x2E
#define SAFE_FREE(p) free(bptr); bptr = NULL;
#define chkgoto(func, var, retvar, label) \
  if (!(var))                             \
  {                                       \
    perror(func);                         \
    retvar = -1;                          \
    goto label;                           \
  }

unsigned char *bptr;

size_t fsize(const char *fn);
int bpull(FILE *fp, unsigned char *b, size_t s);

int main(int argc, char *argv[])
{
  char *fn = NULL;
  FILE *fp = NULL;
  int ret = 0, r;
  size_t s, i;

  if (argc != 2)
  {
    fprintf(stderr, "Usage: ./xcat <file>\n");
    return -1;
  }

  fn = argv[1];
  s = fsize(fn);
  chkgoto("fsize", s, ret, end_label)

  fp = fopen(fn, "rb");
  chkgoto("fopen", fp, ret, end_label)

  bptr = malloc(s);
  chkgoto("malloc", bptr, ret, end_label)

  r = bpull(fp, bptr, s);
  chkgoto("bpull", r, ret, end_label)

  for (i = 0; i < s; ++i)
    bptr[i] ^= KEY;

  fwrite(bptr, 1, s, stdout);

  end_label:
  if (fp) fclose(fp);
  if (bptr) { SAFE_FREE(bptr) }

  return ret;
}

size_t fsize(const char *fn)
{
  struct stat st;
  return (stat(fn, &st) == 0) ? st.st_size : 0;
}

int bpull(FILE *fp, unsigned char *b, size_t s)
{
  return ((s - 1) == fread(b, 1, s, fp)) ? 0 : -1;
}
As a reminder too, if you consider the expansion of your macro, using a semi-colon after chkret() is not necessary, you're adding a null statement to your code.

I wouldn't recommend this for any kind of secure encryption though. This isn't even a cyclical xor, it's just a single one byte key xor.

[Polyphony]

I'm 100% sure xor is not an operator keyword in C.  (maybe in c++?).  Also, macros don't honor types, that's why I'm checking whether or not it's in between 0 and 255, even if I only pass an unsigned char through it.  Also, sizeof(char) is technically architecture-dependent; however, I do think stylistically using

Code: [Select]

malloc(sizeof(type)*count) both looks better and is easier to read.  The variable "fn" on line 21 doesn't go unused, I set it to equal argv[1].  Finally, chkret isn't supposed to free any memory, the only thing it's supposed to do is check the return values!  Lol, all allocated memory gets freed at the end of main.

I don't know why I went through and explained all of this, but don't think I don't welcome constructive criticism like this.  In fact, I quite like explaining some of the design decisions I made.  Especially with semi-old code such as this.

[ArkPhaze]

Ahh, I assumed C++ for some reason.

Also, macros don't honor types, that's why I'm checking whether or not it's in between 0 and 255

I know, it's just a preprocessor replacement, but what's the point? You use a datatype that will never cause issues with reducing your conditional. I don't even see the point behind the macro, it's ONLY used once... Just put the code where it needs to be and don't rely on the preprocessor, and then the programmer can see the intention of the code in the place it needs to be.

There's nothing technical about sizeof(char), it's guaranteed to be 1 by the standard.

I also know that memory is reclaimed by the operating system but that's a lazy and improper way to be thinking about your code. If that was the case, there would never exist a free() function.

[Polyphony]

Well, as much as I hate to admit it , you're correct about sizeof(char), it's definitely equal to one according to the standard.*  I see what you mean about the chkret macro now, not freeing what it should, good catch.

*I took some time to do a little more research into the subject and even if a "char" is 32 bits, sizeof(char) will always return 1.  You can also find the number of bits contained in a char by looking at CHAR_BITS in <limits.h>. Source: (ctrl+f) for section 6.5.3.4

[ArkPhaze]

Well, as much as I hate to admit it , you're correct about sizeof(char), it's definitely equal to one according to the standard.*  I see what you mean about the chkret macro now, not freeing what it should, good catch.

*I took some time to do a little more research into the subject and even if a "char" is 32 bits, sizeof(char) will always return 1.  You can also find the number of bits contained in a char by looking at CHAR_BITS in <limits.h>. Source: (ctrl+f) for section 6.5.3.4

I know... And it's not CHAR_BITS, it's CHAR_BIT. Although, look at sizeof('x'), which is 4 even if CHAR_BIT is 8 too. There's a big difference between the way char literals are interpreted and the way the char datatype by the standard is supposed to behave and work, but there are good reasons for it too, and it doesn't really affect the programmer in any way as long as they know what they are doing..