Fix some security and memory issue

[andreabac3]

This is a small commit that fixes some memory and security issues. I have checked the return values of the mallocs as dynamic memory allocation can fail. Very often memory is not freed.

[AngledLuffa]

But sizeof(char) should always equal 1 regardless of the compiler. It looks very jarring.

https://en.wikipedia.org/wiki/Sizeof

In general the commits are helpful, so thank you!

On Mon, Feb 10, 2020 at 12:17 PM Andrea Bacciu notifications@github.com wrote:

@andreabac3 commented on this pull request.

In src/glove.c https://github.com/stanfordnlp/GloVe/pull/167#discussion_r377293380:

 save_gradsq_file = malloc(sizeof(char) * MAX_STRING_LENGTH);

• if (NULL == save_gradsq_file){

I just committed that fix the missing free. it is always better to use sizeof instead of using constants since the result of the sizeof function depends on the implementation of the compiler and architecture and for clarity of code I would prefer to keep sizeof.

If my commits are helpful, I will continue to investigate it. In the coming days I would like to continue providing support and fixes.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/stanfordnlp/GloVe/pull/167?email_source=notifications&email_token=AA2AYWPCDY23IA2XGJNRZULRCGY7NA5CNFSM4KSTXWUKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCU53AKQ#discussion_r377293380, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA2AYWMH55V57VNJ236KRR3RCGY7NANCNFSM4KSTXWUA .

[andreabac3]

Again, possibly out of date knowledge - isn't free(NULL) supposed to be safe? I do not believe this is necessary.

Sorry you have right, we must set to null each pointer after his free. In order to avoid the re-use after free.

[AngledLuffa]

Thanks for the changes. What I did was incorporated the fin == NULL check and just pre-allocated the buffers instead of allocating them using malloc. Seemed cleaner that way.

[andreabac3]

pre-allocated buffer is the best solution. If you don't mind, you can merge the new pull. In these days I will continue to work on it.