Обсуждение: Patch for snprintf problem (bug #1000650)
Hello, I found the problem in snprintf on linux (maybe another unix) boxes in info.c (CVS HEAD). The problematic part is something like: buf = "text"; snprintf(buf,size,"%s append",buf); buf = "text append" on Windows (MS VC compiler) buf = " append" on linux (gcc compiler) I solve it this way (main idea): snprintf(buf + strlen(buf), " append"); There are more parameters (char *, int, ...) in real usage. More details is here: http://pgfoundry.org/tracker/index.php?func=detail&aid=1000650&group_id=1000125&atid=538 The patch is created againist CVS. Comments are welcome Luf
Вложения
> I solve it this way (main idea): > snprintf(buf + strlen(buf), " append"); Ugh typo mistake: snprintf(buf + strlen(buf), sizeof(buf), " append"); My hands are faster than my head :-( There is buffer overrun erron becouse I have to decrease sizeof(buf) with strlen(buf). So I attach second try of this patch againist CVS HEAD. Comments are welcome Luf
Вложения
Ludek Finstrle wrote: > Hello, > > I found the problem in snprintf on linux (maybe another unix) boxes > in info.c (CVS HEAD). The problematic part is something like: > > buf = "text"; > snprintf(buf,size,"%s append",buf); > > buf = "text append" on Windows (MS VC compiler) > buf = " append" on linux (gcc compiler) > > I solve it this way (main idea): > snprintf(buf + strlen(buf), " append"); Hmm bad news. If so, it may be better to use the sequence like the following for example. char *query_ptr; size_t bufsize_res; int slen; /* Initialize */ query_ptr = columns_query; bufsize_res = sizeof(columns_query); if (..) { if ((slen = snprintf(query_ptr, bufsize_res, .., )) <= 0) { .. error_handling .. } query_ptr += slen; bufsize_res -= slen; } ... regards, Hiroshi Inoue
> > buf = "text"; > > snprintf(buf,size,"%s append",buf); > > > > buf = "text append" on Windows (MS VC compiler) > > buf = " append" on linux (gcc compiler) > > > > I solve it this way (main idea): > > snprintf(buf + strlen(buf), " append"); > > Hmm bad news. > If so, it may be better to use the sequence like the following for example. > > char *query_ptr; > size_t bufsize_res; > int slen; > > /* Initialize */ > query_ptr = columns_query; > bufsize_res = sizeof(columns_query); > > if (..) > { > if ((slen = snprintf(query_ptr, bufsize_res, .., )) <= 0) > { > .. error_handling .. > } > query_ptr += slen; > bufsize_res -= slen; > } > > ... I'm not sure. There are a lot of strcat, my_strcat, schema_strcat calls between snprintf. There is only one place where snprintf could be called after another snprintf. BTW it's something what snprintf_addlen is doing. Or am I miss something? I do the patch with minimal changes in mind. In my opinion strcat may be faster in joining more char *. Regards, Luf
> > > buf = "text"; > > > snprintf(buf,size,"%s append",buf); > > > > > > buf = "text append" on Windows (MS VC compiler) > > > buf = " append" on linux (gcc compiler) > > > > > > I solve it this way (main idea): > > > snprintf(buf + strlen(buf), " append"); > > > > Hmm bad news. > > If so, it may be better to use the sequence like the following for example. > > > > char *query_ptr; > > size_t bufsize_res; > > int slen; > > > > /* Initialize */ > > query_ptr = columns_query; > > bufsize_res = sizeof(columns_query); > > > > if (..) > > { > > if ((slen = snprintf(query_ptr, bufsize_res, .., )) <= 0) > > { > > .. error_handling .. > > } > > query_ptr += slen; > > bufsize_res -= slen; > > } > > > > ... > > I'm not sure. There are a lot of strcat, my_strcat, schema_strcat calls > between snprintf. There is only one place where snprintf could be called > after another snprintf. I change the snprintf_addlen to snprintf_len as you mentioned. Third try of patch attached. Comments are welcome Luf
> I change the snprintf_addlen to snprintf_len as you mentioned. > > Third try of patch attached. I forgot attach the patch. Luf
Вложения
Fri, Jun 09, 2006 at 02:13:23PM +0200, Ludek Finstrle napsal(a): > > > > buf = "text"; > > > > snprintf(buf,size,"%s append",buf); > > > > > > > > buf = "text append" on Windows (MS VC compiler) > > > > buf = " append" on linux (gcc compiler) > > > > > > > > I solve it this way (main idea): > > > > snprintf(buf + strlen(buf), " append"); > > > > > > Hmm bad news. > > > If so, it may be better to use the sequence like the following for example. > > > > > > char *query_ptr; > > > size_t bufsize_res; > > > int slen; > > > > > > /* Initialize */ > > > query_ptr = columns_query; > > > bufsize_res = sizeof(columns_query); > > > > > > if (..) > > > { > > > if ((slen = snprintf(query_ptr, bufsize_res, .., )) <= 0) > > > { > > > .. error_handling .. > > > } > > > query_ptr += slen; > > > bufsize_res -= slen; > > > } > > > > > > ... > > > > I'm not sure. There are a lot of strcat, my_strcat, schema_strcat calls > > between snprintf. There is only one place where snprintf could be called > > after another snprintf. > > I change the snprintf_addlen to snprintf_len as you mentioned. I take a look at all snprintf in psqlodbc an I find one more place where it's problematic. If we accept snprintf_add I change one call to it. Additional patch attached, comments are welcome Luf
Вложения
> > > > buf = "text"; > > > > snprintf(buf,size,"%s append",buf); > > > > > > > > buf = "text append" on Windows (MS VC compiler) > > > > buf = " append" on linux (gcc compiler) > > > > > > > > I solve it this way (main idea): > > > > snprintf(buf + strlen(buf), " append"); > > > > > > Hmm bad news. > > > If so, it may be better to use the sequence like the following for example. > > > > > > char *query_ptr; > > > size_t bufsize_res; > > > int slen; > > > > > > /* Initialize */ > > > query_ptr = columns_query; > > > bufsize_res = sizeof(columns_query); > > > > > > if (..) > > > { > > > if ((slen = snprintf(query_ptr, bufsize_res, .., )) <= 0) > > > { > > > .. error_handling .. > > > } > > > query_ptr += slen; > > > bufsize_res -= slen; > > > } > > > > > > ... > > > > I'm not sure. There are a lot of strcat, my_strcat, schema_strcat calls > > between snprintf. There is only one place where snprintf could be called > > after another snprintf. > > I change the snprintf_addlen to snprintf_len as you mentioned. > > Third try of patch attached. I make patch againist CVS after yours huge commit. What's your opinion? Could we fix linux bug? Or do you prefer ifdef or what's your idea about change these lines: snprintf(buf,bufsize,"%s adding 1",buf); strcat(buf," adding 2"); snprintf(buf,bufsize,"%s adding 3",buf); I think it's hard to maintain code you mentioned above. This calls are only in catalog information (relatively short query - used not so often). Patch attached contains previous psqlodbc_snprintf_3try and psqlodbc_snprintf_next. Regards, Luf
Вложения
Ludek Finstrle wrote: > > I make patch againist CVS after yours huge commit. What's your opinion? > Is the second parameter of snprintf_add needed ? Aren't the parameter values always strlen(the first parameter) ? Is snprintf_len needed instead of snprintf ? Though the current code ignores snprintf errors, it's simply my negligence.. regards, Hiroshi Inoue
Tue, Jun 13, 2006 at 11:12:58PM +0900, Hiroshi Inoue wrote: > Ludek Finstrle wrote: > > > > I make patch againist CVS after yours huge commit. What's your opinion? > > > > Is the second parameter of snprintf_add needed ? > Aren't the parameter values always strlen(the first parameter) ? You're right. I think more about it and "add" means add to the end so I changed the patch as you pointed. > Is snprintf_len needed instead of snprintf ? > Though the current code ignores snprintf errors, it's simply > my negligence.. I'm voting for keeping safer snprintf_len. But I can change it if you wish. New patch attached. Regards, Luf
Вложения
Ludek Finstrle wrote: > Tue, Jun 13, 2006 at 11:12:58PM +0900, Hiroshi Inoue wrote: > >> Ludek Finstrle wrote: >> >>> I make patch againist CVS after yours huge commit. What's your opinion? >>> >>> >> Is the second parameter of snprintf_add needed ? >> Aren't the parameter values always strlen(the first parameter) ? >> > > You're right. I think more about it and "add" means add to the end > so I changed the patch as you pointed. > > >> Is snprintf_len needed instead of snprintf ? >> Though the current code ignores snprintf errors, it's simply >> my negligence.. >> > > I'm voting for keeping safer snprintf_len. But I can change it if > you wish. > > New patch attached. > OK please commit it. regards, Hiroshi Inoue
> OK please commit it. Patch commited. Thanks, Luf