Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Functionality

Error: input(stdin, 0) could quickly return NULL as code incorrectly assumes a NULL return only means out-of-memory.

// if (!input_str) return NULL;
if (!input_str && size > 0) return NULL;

Error: Memory leak @Daniel Jour@Daniel Jour. Should realloc() fail, code should free input_str.

        size = size ? size*2 : 1;
        void *new_ptr = realloc(input_str, size);
        if (new_ptr == NULL) {
          free(input_str);
          return NULL;
        }

Weakness: Code lacks EOF detection. If only '\n' or EOF is read, both cases return a string of "". Code needs to differentiate. Various approaches exists. With this code, recommend returning NULL when only EOF was read. Calling code could distinguish between OOM and EOF occurrence, by calling feof()/ferror().

Memory leak with return realloc(input_str, len); on rare failure.

 // return realloc(input_str, len);
 void *new_ptr = realloc(input_str, len);
 return new_ptr ? new_ptr : input_ptr;

Soap box

Disagree with unlimited input strategy as it gives external input the ability for code to consume insane amount of resources without control - ripe for hackers. Better to pass in an upper bound.

Design

size parameter seems of little use. It only provides an initial size and is ignored for subsequent re-allocations. Recommend removing it. Just start with some small size.

A better use would be to use size_t *size_read to report that number of characters read. This has an advantage over using strlen() on the return value as 1) the length does not need O(n) computation and 2) handles the corner case should code read null characters which strlen() will not detect.

IMO, code should not attempt to read unless the needed memory has first been successfully allocated. Also on allocation failure, perhaps it would be better to return the partial read line. Of course, conveying that information is another issue. IAC, this allows the calling routine more control on error handling.

Performance

Use exponential growth in re-allocation rather than linear. This is well answered by @mleyfman@mleyfman

With exponentiation strategy, it makes even more sense then to do a final "right-size " re-allocation, which OP did. With linear +16 re-allocation, it has reduced value to the point that it is unnecessary.

Minor: Check against the more common '\n' first.

// while ((c = fgetc(in)) != EOF && c != '\n')
while ((c = fgetc(in)) != '\n' && c != EOF)

Functionality

Error: input(stdin, 0) could quickly return NULL as code incorrectly assumes a NULL return only means out-of-memory.

// if (!input_str) return NULL;
if (!input_str && size > 0) return NULL;

Error: Memory leak @Daniel Jour. Should realloc() fail, code should free input_str.

        size = size ? size*2 : 1;
        void *new_ptr = realloc(input_str, size);
        if (new_ptr == NULL) {
          free(input_str);
          return NULL;
        }

Weakness: Code lacks EOF detection. If only '\n' or EOF is read, both cases return a string of "". Code needs to differentiate. Various approaches exists. With this code, recommend returning NULL when only EOF was read. Calling code could distinguish between OOM and EOF occurrence, by calling feof()/ferror().

Memory leak with return realloc(input_str, len); on rare failure.

 // return realloc(input_str, len);
 void *new_ptr = realloc(input_str, len);
 return new_ptr ? new_ptr : input_ptr;

Soap box

Disagree with unlimited input strategy as it gives external input the ability for code to consume insane amount of resources without control - ripe for hackers. Better to pass in an upper bound.

Design

size parameter seems of little use. It only provides an initial size and is ignored for subsequent re-allocations. Recommend removing it. Just start with some small size.

A better use would be to use size_t *size_read to report that number of characters read. This has an advantage over using strlen() on the return value as 1) the length does not need O(n) computation and 2) handles the corner case should code read null characters which strlen() will not detect.

IMO, code should not attempt to read unless the needed memory has first been successfully allocated. Also on allocation failure, perhaps it would be better to return the partial read line. Of course, conveying that information is another issue. IAC, this allows the calling routine more control on error handling.

Performance

Use exponential growth in re-allocation rather than linear. This is well answered by @mleyfman

With exponentiation strategy, it makes even more sense then to do a final "right-size " re-allocation, which OP did. With linear +16 re-allocation, it has reduced value to the point that it is unnecessary.

Minor: Check against the more common '\n' first.

// while ((c = fgetc(in)) != EOF && c != '\n')
while ((c = fgetc(in)) != '\n' && c != EOF)

Functionality

Error: input(stdin, 0) could quickly return NULL as code incorrectly assumes a NULL return only means out-of-memory.

// if (!input_str) return NULL;
if (!input_str && size > 0) return NULL;

Error: Memory leak @Daniel Jour. Should realloc() fail, code should free input_str.

        size = size ? size*2 : 1;
        void *new_ptr = realloc(input_str, size);
        if (new_ptr == NULL) {
          free(input_str);
          return NULL;
        }

Weakness: Code lacks EOF detection. If only '\n' or EOF is read, both cases return a string of "". Code needs to differentiate. Various approaches exists. With this code, recommend returning NULL when only EOF was read. Calling code could distinguish between OOM and EOF occurrence, by calling feof()/ferror().

Memory leak with return realloc(input_str, len); on rare failure.

 // return realloc(input_str, len);
 void *new_ptr = realloc(input_str, len);
 return new_ptr ? new_ptr : input_ptr;

Soap box

Disagree with unlimited input strategy as it gives external input the ability for code to consume insane amount of resources without control - ripe for hackers. Better to pass in an upper bound.

Design

size parameter seems of little use. It only provides an initial size and is ignored for subsequent re-allocations. Recommend removing it. Just start with some small size.

A better use would be to use size_t *size_read to report that number of characters read. This has an advantage over using strlen() on the return value as 1) the length does not need O(n) computation and 2) handles the corner case should code read null characters which strlen() will not detect.

IMO, code should not attempt to read unless the needed memory has first been successfully allocated. Also on allocation failure, perhaps it would be better to return the partial read line. Of course, conveying that information is another issue. IAC, this allows the calling routine more control on error handling.

Performance

Use exponential growth in re-allocation rather than linear. This is well answered by @mleyfman

With exponentiation strategy, it makes even more sense then to do a final "right-size " re-allocation, which OP did. With linear +16 re-allocation, it has reduced value to the point that it is unnecessary.

Minor: Check against the more common '\n' first.

// while ((c = fgetc(in)) != EOF && c != '\n')
while ((c = fgetc(in)) != '\n' && c != EOF)
added 5 characters in body
Source Link
chux
  • 33.6k
  • 2
  • 38
  • 92

Functionality

Error: input(stdin, 0) could quickly return NULL as code incorrectly assumes a NULL return only means out-of-memory.

// if (!input_str) return NULL;
if (!input_str && size > 0) return NULL;

Error: Memory leak @Daniel Jour. Should realloc() fail, code should free input_str.

        size = size ? size*2 : 1;
        void *new_ptr = realloc(input_str, size);
        if (new_ptr == NULL) {
          free(input_str);
          return NULL;
        }

Weakness: Code lacks EOF detection. If only '\n' or EOF is read, both cases return a string of "". Code needs to differentiate. Various approaches exists. With this code, recommend returning NULL when only EOF was read. Calling code could distinguish between OOM and EOF occurrence, by calling feof()/ferror().

Memory leak with return realloc(input_str, len); on rare failure.

 // return realloc(input_str, len);
 void *new_ptr = realloc(input_str, len);
 return new_ptr ? new_ptr : input_ptr;

Soap box

Disagree with unlimited input strategy as it gives external input the ability for code to consume insane amount of resources without control - ripe for hackers. Better to pass in an upper bound.

Design

size parameter seems of little use. It only provides an initial size and is ignored for subsequent re-allocations. Recommend removing it. Just start with some small size.

A better use would be to use size_t *size_read to report that number of characters read. This has an advantage over using strlen() on the return value as 1) the length does not need O(n) computation and 2) handles the corner case should code read null characters which strlen() will not detect.

IMO, code should not attempt to read unless the needed memory has first been successfully allocated. Also on allocation failure, perhaps it would be better to return the partial read line. Of course, conveying that information is another issue. IAC, this allows the calling routine more control on error handling.

Performance

Use exponential growth in re-allocation rather than linear. This is well answered by @mleyfman

With exponentiation strategy, it makes even more sense then to do a final "right-size " re-allocation, which OP did. With linear +16 re-allocation, it has reduced value to the point that it is unnecessary.

Minor: Check against the more common '\n' first.

// while ((c = fgetc(in)) != EOF && c != '\n')
while ((c = fgetc(in)) != '\n' && c != EOF)

Functionality

Error: input(stdin, 0) could quickly return NULL as code incorrectly assumes a NULL return means out-of-memory.

// if (!input_str) return NULL;
if (!input_str && size > 0) return NULL;

Error: Memory leak @Daniel Jour. Should realloc() fail, code should free input_str.

        size = size ? size*2 : 1;
        void *new_ptr = realloc(input_str, size);
        if (new_ptr == NULL) {
          free(input_str);
          return NULL;
        }

Weakness: Code lacks EOF detection. If only '\n' or EOF is read, both cases return a string of "". Code needs to differentiate. Various approaches exists. With this code, recommend returning NULL when only EOF was read. Calling code could distinguish between OOM and EOF occurrence, by calling feof()/ferror().

Memory leak with return realloc(input_str, len); on rare failure.

 // return realloc(input_str, len);
 void *new_ptr = realloc(input_str, len);
 return new_ptr ? new_ptr : input_ptr;

Soap box

Disagree with unlimited input strategy as it gives external input the ability for code to consume insane amount of resources without control - ripe for hackers. Better to pass in an upper bound.

Design

size parameter seems of little use. It only provides an initial size and is ignored for subsequent re-allocations. Recommend removing it. Just start with some small size.

A better use would be to use size_t *size_read to report that number of characters read. This has an advantage over using strlen() on the return value as 1) the length does not need O(n) computation and 2) handles the corner case should code read null characters which strlen() will not detect.

IMO, code should not attempt to read unless the needed memory has first been successfully allocated. Also on allocation failure, perhaps it would be better to return the partial read line. Of course, conveying that information is another issue. IAC, this allows the calling routine more control on error handling.

Performance

Use exponential growth in re-allocation rather than linear. This is well answered by @mleyfman

With exponentiation strategy, it makes even more sense then to do a final "right-size " re-allocation, which OP did. With linear +16 re-allocation, it has reduced value to the point that it is unnecessary.

Minor: Check against the more common '\n' first.

// while ((c = fgetc(in)) != EOF && c != '\n')
while ((c = fgetc(in)) != '\n' && c != EOF)

Functionality

Error: input(stdin, 0) could quickly return NULL as code incorrectly assumes a NULL return only means out-of-memory.

// if (!input_str) return NULL;
if (!input_str && size > 0) return NULL;

Error: Memory leak @Daniel Jour. Should realloc() fail, code should free input_str.

        size = size ? size*2 : 1;
        void *new_ptr = realloc(input_str, size);
        if (new_ptr == NULL) {
          free(input_str);
          return NULL;
        }

Weakness: Code lacks EOF detection. If only '\n' or EOF is read, both cases return a string of "". Code needs to differentiate. Various approaches exists. With this code, recommend returning NULL when only EOF was read. Calling code could distinguish between OOM and EOF occurrence, by calling feof()/ferror().

Memory leak with return realloc(input_str, len); on rare failure.

 // return realloc(input_str, len);
 void *new_ptr = realloc(input_str, len);
 return new_ptr ? new_ptr : input_ptr;

Soap box

Disagree with unlimited input strategy as it gives external input the ability for code to consume insane amount of resources without control - ripe for hackers. Better to pass in an upper bound.

Design

size parameter seems of little use. It only provides an initial size and is ignored for subsequent re-allocations. Recommend removing it. Just start with some small size.

A better use would be to use size_t *size_read to report that number of characters read. This has an advantage over using strlen() on the return value as 1) the length does not need O(n) computation and 2) handles the corner case should code read null characters which strlen() will not detect.

IMO, code should not attempt to read unless the needed memory has first been successfully allocated. Also on allocation failure, perhaps it would be better to return the partial read line. Of course, conveying that information is another issue. IAC, this allows the calling routine more control on error handling.

Performance

Use exponential growth in re-allocation rather than linear. This is well answered by @mleyfman

With exponentiation strategy, it makes even more sense then to do a final "right-size " re-allocation, which OP did. With linear +16 re-allocation, it has reduced value to the point that it is unnecessary.

Minor: Check against the more common '\n' first.

// while ((c = fgetc(in)) != EOF && c != '\n')
while ((c = fgetc(in)) != '\n' && c != EOF)
Source Link
chux
  • 33.6k
  • 2
  • 38
  • 92

Functionality

Error: input(stdin, 0) could quickly return NULL as code incorrectly assumes a NULL return means out-of-memory.

// if (!input_str) return NULL;
if (!input_str && size > 0) return NULL;

Error: Memory leak @Daniel Jour. Should realloc() fail, code should free input_str.

        size = size ? size*2 : 1;
        void *new_ptr = realloc(input_str, size);
        if (new_ptr == NULL) {
          free(input_str);
          return NULL;
        }

Weakness: Code lacks EOF detection. If only '\n' or EOF is read, both cases return a string of "". Code needs to differentiate. Various approaches exists. With this code, recommend returning NULL when only EOF was read. Calling code could distinguish between OOM and EOF occurrence, by calling feof()/ferror().

Memory leak with return realloc(input_str, len); on rare failure.

 // return realloc(input_str, len);
 void *new_ptr = realloc(input_str, len);
 return new_ptr ? new_ptr : input_ptr;

Soap box

Disagree with unlimited input strategy as it gives external input the ability for code to consume insane amount of resources without control - ripe for hackers. Better to pass in an upper bound.

Design

size parameter seems of little use. It only provides an initial size and is ignored for subsequent re-allocations. Recommend removing it. Just start with some small size.

A better use would be to use size_t *size_read to report that number of characters read. This has an advantage over using strlen() on the return value as 1) the length does not need O(n) computation and 2) handles the corner case should code read null characters which strlen() will not detect.

IMO, code should not attempt to read unless the needed memory has first been successfully allocated. Also on allocation failure, perhaps it would be better to return the partial read line. Of course, conveying that information is another issue. IAC, this allows the calling routine more control on error handling.

Performance

Use exponential growth in re-allocation rather than linear. This is well answered by @mleyfman

With exponentiation strategy, it makes even more sense then to do a final "right-size " re-allocation, which OP did. With linear +16 re-allocation, it has reduced value to the point that it is unnecessary.

Minor: Check against the more common '\n' first.

// while ((c = fgetc(in)) != EOF && c != '\n')
while ((c = fgetc(in)) != '\n' && c != EOF)