

Show HN: Progressbar - A C library for displaying command-line progress bars - Doches
https://github.com/doches/progressbar

======
_RPM
Consider checking the return value of malloc (e.g:
[https://github.com/doches/progressbar/blob/master/lib/progre...](https://github.com/doches/progressbar/blob/master/lib/progressbar.c#L22))
You're setting users/yourself up for a disaster if you're assuming it will
never fail.

The standard [0] states that the size of `char` will be 1 byte. So there is no
point in doing the sizeof operator on char. (e.g:
[https://github.com/doches/progressbar/blob/master/lib/progre...](https://github.com/doches/progressbar/blob/master/lib/progressbar.c#L28))

6.5.3.4 - Paragraph 3

> The sizeof operator... (page 80)

[0] [http://www.open-
std.org/jtc1/sc22/wg14/www/docs/n1124.pdf](http://www.open-
std.org/jtc1/sc22/wg14/www/docs/n1124.pdf)

~~~
danieltillett
How often is knowing malloc has failed useful? You going to segfault either
way.

I always use sizeof with char just to stay consistent - if you don’t you will
get caught out one day when you change something and forget to add in the
needed new sizeof.

~~~
_RPM
It's _very_ important to know if it failed if you don't want to introduce
undefined behavior. With undefined behavior, _anything_ can happen, (e.g.
arbitrary code execution). But let me make it clear: malloc failing doesn't
mean that undefined behavior will happen. However, pretending like malloc
didn't fail when it really did will most likely get you undefined behavior.
You're not going to segfault either way if you handle things correctly.
Consider a web server running out of memory to allocate a request structure,
do you think it is a good idea to just let it SIGSEV?

~~~
aivarsk
I have some questions:

Have you experienced malloc failing a lot? How do you test to ensure error
handling code paths work as expected?

Have you read 'notes' section of Linux malloc manpage? It says:

By default, Linux follows an optimistic memory allocation strategy. This means
that when malloc() returns non-NULL there is no guarantee that the memory
really is available. In case it turns out that the system is out of memory,
one or more processes will be killed by the OOM killer.

~~~
danieltillett
I have never knowingly experienced malloc failing in any of my code which is
why I asked if it is something to worry about in practice. Once you run out of
memory things start dying or the system locks up hard.

Edit. It is impossible for malloc to fail (at least on linux) [1]. Seems
rather pointless checking for something that can’t happen.

1\. [https://scvalex.net/posts/6/](https://scvalex.net/posts/6/)

~~~
dalke
That link shows that malloc failed, just not due to exceeding available RAM.
At the end it says "malloc finally failed because of address space
exhaustion."

~~~
danieltillett
If the memory was actually being used it would have crashed long before that.
The test was kind of artificial.

~~~
dalke
Absolutely. I was pointing out that "It is impossible for malloc to fail" is,
according to the linked page, an incorrect summary.

~~~
danieltillett
It is still pointless checking if malloc fails on linux :)

Actually the real issue here is that you should be aware that on some systems
malloc might fail and not segfault.

------
natch
Everybody, please look at this Readme file. It starts with a section called
"What is this thing?"

That is a fantastic example of something everyone should have in their OSS
projects. A simple section at the top of the main doc that states what the
thing is. Seems obvious, but I only mention it because it's so rarely done.

------
brobinson
A screenshot in the readme of what it looks like would go a long way.

~~~
SloopJon
The output of the demo program, FWIW:

    
    
        $ ./demo
        Smooth |=====================================================| ETA: 0h00m06s
        Three Second Task with a long label |========================| ETA: 0h00m03s
        Fast |=======================================================| ETA: 0h00m02s
        Custom <.....................................................> ETA: 0h00m06s
        Indeterminate:                                                           0:00:03
        Status bar with a really long label:                                     0:00:01
        Custom:                                                                  0:00:03

------
AndyKelley
The API doesn't let you set the progress, you must know in advance how many
steps there are and then call the increment function that exact number of
steps. This is not conducive to, for example, a network transfer where you
have some total number of bytes, and then you receive a packet of some
arbitrary size and you want to set the progress to a percent.

------
khanan
As mentioned earlier, pv have been around for ages and does this "the right
way" (tm). Also, as an exercise, spot the potential buffer overflow in this
source in less than 10 seconds for extra credits!

~~~
_RPM
[https://github.com/doches/progressbar/blob/master/lib/progre...](https://github.com/doches/progressbar/blob/master/lib/progressbar.c#L28:L30)

The correct solution would be a) check the length of format, and if it less
than or equal to 4 but greater than 0, then copy the bytes, else, don't... I
guess? Why is 4 such an arbitrary chosen number?

    
    
        // new->format is allocated to 4 bytes
        size_t len = strlen(format);
        if(len > 0 && len <= 4) {
           memcpy(new->format, format, len); //i to < 4
           new->format[3] = '\0';
        }
    

Or it just be better to allocate the new->length to the return value of
strlen(format) + 1.

~~~
danieldk
Maybe I am not awake yet (just rolled out of bed), but if new->format is
allocated to 4 bytes, then

    
    
        new->format[4] = '\0';
    

is out of bounds, no?

~~~
_RPM
Yes it was. Good catch. The size of the buffer is 4, so the last byte would be
located at 4-1 = index 3. Because we're 0 based...Wow can't believe I did
that. it's 12:30 AM here and I've had a few beers tonight.

It's common to allocate your size needed plus 1 for the NULL byte. But this
programmer only allocates 4 bytes.. with no thought of the NULL byte at the
end.

------
sigil
See also the `pv` program:
[http://www.ivarch.com/programs/pv.shtml](http://www.ivarch.com/programs/pv.shtml)

------
blt
Beware weaving progress bar code into computationally heavy routines. As soon
as you want to start multithreading, it gets tricky. The worst is a
ProgressBar singleton in a big C++ GUI program. If I'm going to bother using a
library for progress bars, I want it to deal with all those work of combining
multiple threads' progress, i.e. it has a queue, knows each thread's estimated
share of the total work, and all the threads push progress messages on the
queue.

------
Exuma
Can we get a screenshot?

------
eps
Needs work ;)

    
    
      progressbar * p;
      p = progressbar_new_with_format(..., ..., "whoops");
    

... yields a buffer overflow. And why do you malloc _format_ to begin with if
it's always 3 chars in size?

------
copperx
I'm a bit surprised that this didn't exist before.

