Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix buffer overrun in tabs[MAX_NESTS] by adding space for \0 terminator #729

Merged
merged 2 commits into from
Dec 21, 2017

Conversation

mingwandroid
Copy link
Contributor

.. also speed it up by not using strcat when direct array access works just fine.

In the Anaconda Distribution we build on macOS with -fstack-protector-strong. Here is the stack trace I got:

1  __pthread_kill                         0x7fff58d86e3e 
2  pthread_kill                           0x7fff58ec5150 
3  abort                                  0x7fff58ce3312 
4  abort_report_np                        0x7fff58ce3485 
5  __chk_fail                             0x7fff58d07c2d 
6  __chk_fail_overflow                    0x7fff58d07bfd 
7  __strcat_chk                           0x7fff58d07fb9 
8  rec_print_metadata  nc4internal.c 1484 0x1001081e0    
9  rec_print_metadata  nc4internal.c 1561 0x1001085bb    
10 rec_print_metadata  nc4internal.c 1561 0x1001085bb    
11 rec_print_metadata  nc4internal.c 1561 0x1001085bb    
12 rec_print_metadata  nc4internal.c 1561 0x1001085bb    
13 rec_print_metadata  nc4internal.c 1561 0x1001085bb    
14 rec_print_metadata  nc4internal.c 1561 0x1001085bb    
15 rec_print_metadata  nc4internal.c 1561 0x1001085bb    
16 rec_print_metadata  nc4internal.c 1561 0x1001085bb    
17 rec_print_metadata  nc4internal.c 1561 0x1001085bb    
18 rec_print_metadata  nc4internal.c 1561 0x1001085bb    
19 nc4_open_file       nc4file.c     2350 0x1000fc2f7    
20 NC4_open            nc4file.c     2867 0x1000fbe05    
21 NC_open             dfile.c       1980 0x1000bcb18    
22 nc_open             dfile.c       652  0x1000bc8eb    
23 main                ncdump.c      2296 0x1000037f2    
24 start                                  0x7fff58c37115 
25 start                                  0x7fff58c37115 

Please consider my patch for inclusion, if you have any problems with it then please let me know.

.. also speed it up by not using strcat when direct array access works just fine.
@CLAassistant
Copy link

CLAassistant commented Dec 20, 2017

CLA assistant check
All committers have signed the CLA.

char *dims_string = NULL;
char temp_string[10];
int t, retval, d, i;

/* Come up with a number of tabs relative to the group. */
for (t = 0; t < tab_count && t < MAX_NESTS; t++)
strcat(tabs, "\t");
tabs[t] = '\t';
tabs[t] = '\0';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this line, it is not needed. The strcat function is guaranteed to add the null terminator.

Copy link
Contributor Author

@mingwandroid mingwandroid Dec 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the strcat altogether because it is inefficient to perform 55 operations (and 10 function calls) instead of 10 direct writes and no function calls.

edit: small correction

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK that's cool!

@@ -1857,14 +1857,15 @@ rec_print_metadata(NC_GRP_INFO_T *grp, int tab_count)
NC_DIM_INFO_T *dim;
NC_TYPE_INFO_T *type;
NC_FIELD_INFO_T *field;
char tabs[MAX_NESTS] = "";
char tabs[MAX_NESTS+1] = "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@WardF
Copy link
Member

WardF commented Dec 21, 2017

Thank you very much, it looks great. Updating with the changes in master and will merge :).

@WardF WardF merged commit 1cdafd2 into Unidata:master Dec 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants