BorlandTalk.com Borland discussion newsgroups   Archives   FAQ   Search   Memberlist   Usergroups   Register   Profile   Log in to check your private messages   Log in  What is wrong with this code?          BorlandTalk.com Forum Index -> C++ Builder (Language C++) View previous topic :: View next topic   Author Message peter Guest Posted: Tue Mar 01, 2005 9:40 pm    Post subject: What is wrong with this code? i feel very lame that I can't figure out why this code is crashing. char* __stdcall makeClaimHeader(char requestor[], char requestNumber[],char sort1[], char sort2[],char sort3[],char sort4[]){ char headerMaker[100]; char header[87]; char ending = ''; sprintf(headerMaker,"%s%s%s%s%s%s%c",requestor,requestNumber,sort1,sort2,sort3,sort4,ending);//strcpy(destination, source); strcpy(header,headerMaker); return &header; } when i return the 'header' variable, it crashes. the function call looks like this: strcpy(myHeader,makeClaimHeader(claim_header.requestor,claim_header.request_number, claim_header.sort1,claim_header.sort2,claim_header.sort3,claim_header.sort4)); and, in case this helps, this is being called from a DLL. please help me out. thanks Back to top Liz Albin Guest Posted: Tue Mar 01, 2005 9:57 pm    Post subject: Re: What is wrong with this code? On 1 Mar 2005 13:40:42 -0800, peter wrote: Quote: when i return the 'header' variable, it crashes. the function call looks like this: You haven't said how it crashes, but two problems I see are that you're attempting to return a pointer to an array of char rather than the array itself, and that header[] is allocated on the stack, and is a local variable - it's not really available once the function returns. You might be better off with passing the buffer you need, or even using std::string: char* __stdcall makeClaimHeader(char requestor[], char requestNumber[],char sort1[], char sort2[],char sort3[], char sort4[], char header[]){ char headerMaker[100]; char ending = ''; sprintf(headerMaker,"%s%s%s%s%s%s%c",requestor,requestNumber,sort1,sort2,sort3,sort4,ending);//strcpy(destination, source); strcpy(header,headerMaker); return header; } -- Good luck, liz Back to top Hans Galema Guest Posted: Tue Mar 01, 2005 9:59 pm    Post subject: Re: What is wrong with this code? peter wrote: Quote: i feel very lame that I can't figure out why this code is crashing. And then you could even check the contents of the paameters of your call which we cann't. But yes the code is potentially dangerous and contains an error. Quote: char* __stdcall makeClaimHeader(char requestor[], char requestNumber[],char sort1[], char sort2[],char sort3[],char sort4[]){ char headerMaker[100]; char header[87]; Why one buffer 100 and the other 87 ? Are yopu sure there are no buffer overflows ? Do you have CodeGuard enabled ? It would warn. Quote: char ending = ''; sprintf(headerMaker,"%s%s%s%s%s%s%c",requestor,requestNumber,sort1,sort2,sort3,sort4,ending);//strcpy(destination, source); strcpy(header,headerMaker); Why first put it in headerMaker and then copy it to header ? Makes no sence. Quote: return &header; That should have been return header; But even that would not work as you cannot return the addres of a local variable (on the stack) as it is invalid after the call. You could make it valid though by: static char header [87]; Quote: } when i return the 'header' variable, it crashes. the function call looks like this: As explained. Quote: strcpy(myHeader,makeClaimHeader(claim_header.requestor,claim_header.request_number, claim_header.sort1,claim_header.sort2,claim_header.sort3,claim_header.sort4)); This looks like a terrible call. And again you do a copy of the result. Better give your function an extra parameter ( the first one). Make it the address of the buffer where tou put all in. You could do away with both local buffers then. Hans. Back to top Chris Uzdavinis (TeamB) Guest Posted: Tue Mar 01, 2005 9:59 pm    Post subject: Re: What is wrong with this code? "peter" <peterna (AT) attglobla (DOT) net> writes: Quote: i feel very lame that I can't figure out why this code is crashing. char* __stdcall makeClaimHeader(char requestor[], char requestNumber[],char sort1[], char sort2[],char sort3[],char sort4[]){ First of all, the interface to this function is very dangerous because it is impossible for the function to know how many bytes each of those arguments points to. Whenver you pass in a raw buffer you should pass in its length, to have a chance of detecting buffer-overflows before they happen. Quote: char headerMaker[100]; char header[87]; char ending = ''; sprintf(headerMaker,"%s%s%s%s%s%s%c",requestor,requestNumber,sort1,sort2,sort3,sort4,ending);//strcpy(destination, source); You cannot guarantee that you're not writing more data than you have space. sprintf is dangerous to use. Quote: strcpy(header,headerMaker); You know that header not as long as headerMaker, so this is inherently flawed code. Expect buffer-overflows here whenever headerMaker holds more than 87 bytes. Why write into headerMaker only to copy it into header? If you're going to go this route in the first place, why not save the copy by sprintf-ing directly into header? Quote: return &header; } Returning the address of an variable with automatic storage duration is always a serious bug. As the function returns, the stack memory is reclaimed and the buffer invalidated. The caller should pass in a buffer that should hold the output (and should pass in its length.) The function should write the output into that buffer, and ensure that it does not write more than the specified size of the buffer. Quote: when i return the 'header' variable, it crashes. Be thankful, because sometimes those types of serious bugs don't crash. Silent failure and corruption is worse than a nice attention-grabbing crash. Quote: strcpy(myHeader,makeClaimHeader(claim_header.requestor,claim_header.request_number, claim_header.sort1,claim_header.sort2,claim_header.sort3,claim_header.sort4)); Yet another unsafe, slow copy. Copying is slow--it's good to design code to minimize copying data when possible. strcpy is dangerous. At least consider using strncpy instead. - Chris (TeamB); Back to top peter Guest Posted: Tue Mar 01, 2005 10:08 pm    Post subject: thanks! thanks for the response, you have given me ample opportunity to find the error. I was going about it all wrong, and found a much simpler solution. I did not realize that strcpy and sprintf were so dangerous, and i will take that into consideration in the future! thanks once again. Back to top Chris Uzdavinis (TeamB) Guest Posted: Tue Mar 01, 2005 11:21 pm    Post subject: Re: thanks! "peter" <peterna (AT) attglobal (DOT) net> writes: Quote: thanks for the response, you have given me ample opportunity to find the error. I was going about it all wrong, and found a much simpler solution. I did not realize that strcpy and sprintf were so dangerous, and i will take that into consideration in the future! thanks once again. Glad to help. If today wasn't so frantic I'd have softened my tone a bit... Anyway, there are functions snprintf and strncpy which are a little safer, since you can specify a max number of bytes to write. You might consider using std::string to hide all the memory management issues. (But be careful to pass them by reference to functions to avoid gratituous copying. And pass them as const objects whenever you don't intend to change them.) -- Chris (TeamB); Back to top Wiljo Guest Posted: Tue Mar 08, 2005 12:27 pm    Post subject: Re: thanks! Chris Uzdavinis (TeamB) wrote: Quote: "peter" <peterna (AT) attglobal (DOT) net> writes: thanks for the response, you have given me ample opportunity to find the error. I was going about it all wrong, and found a much simpler solution. I did not realize that strcpy and sprintf were so dangerous, and i will take that into consideration in the future! thanks once again. Glad to help. If today wasn't so frantic I'd have softened my tone a bit... Anyway, there are functions snprintf and strncpy which are a little safer, since you can specify a max number of bytes to write. You might consider using std::string to hide all the memory management issues. (But be careful to pass them by reference to functions to avoid gratituous copying. And pass them as const objects whenever you don't intend to change them.) Hello, I am a newbie to this group, and I want to add something as well. I don't think using sprintf or strcpy are so invalid. As long as the buffers are tested beforehand, and they are big enough there is no problem. Using strncpy on the otherhand, as you mentioned, is introducing a very heavy pitfall. This function does *NOT* end the copied string with a terminating ''-character. So using strncpy *MUST* be accompanied by another line which adds this terminating ''-character, otherwise a crash is surely to be expected as soon as the copied buffer is used. It is always better to use std:string compared to basic character handling. But even std:string will use this basic string handling internally, beit with a protection mechanism. Wiljo. Back to top Alan Bellingham Guest Posted: Tue Mar 08, 2005 1:54 pm    Post subject: Re: thanks! Wiljo <invalid (AT) invalid (DOT) address> wrote: Quote: It is always better to use std:string compared to basic character handling. But even std:string will use this basic string handling internally, beit with a protection mechanism. std::string will _not_ (unless terminally broken) use strcpy() or strncpy() equivalents internally. It maintains a length, so embedded nulls do not cause it to terminate early on copies from one std::string to another. (So, it effectively does a memcpy() instead.) The places where embedded nulls will cause a problem with std::string are when creating from a raw char* and not giving it a length (in which case, it has no option but to use a strlen() equivalent on the source), or when you're using the c_str() member to get back the data pointer, and you ignore the size() member. Both are cases where you're interfacing with C-style functions. As far as strncpy() is concerned as opposed to strcpy(), well, it's only a problem if the target buffer is too short. In the first case, you don't get the null appended (but that's easily sorted by passing the actual buffer length - 1 to start with, and setting the last character NULL). In the second case, you get a memory overwrite and, if you're lucky, your program crashes immediately. (If you're not lucky, strange things happen. Strange, difficult to diagnose things. Things that take ages to debug.) I think I prefer the former. (I'd prefer if strncpy always wrote the terminating null, but I can understand the arguments that it doesn't.) Alan Bellingham -- ACCU Conference 2005 - 20-23 April, Randolph Hotel, Oxford, UK Back to top Display posts from previous: All Posts1 Day7 Days2 Weeks1 Month3 Months6 Months1 Year Oldest FirstNewest First         BorlandTalk.com Forum Index -> C++ Builder (Language C++) All times are GMT Page 1 of 1   Jump to: Select a forum C++ Builder Newsgroups----------------C++ BuilderC++ Builder (ActiveX)C++ Builder (Announcements)C++ Builder (Command Line Tools)C++ Builder DatabasesC++ Builder Databases (ADO)C++ Builder Databases (Desktop)C++ Builder Databases (InterBase Express)C++ Builder Databases (Multi-Tier)C++ Builder Databases (SQL Servers)C++ Builder (Graphics)C++ Builder (IDE)C++ Builder (Internet)C++ Builder (Internet Socket)C++ Builder (Internet Web)C++ Builder (Jobs)C++ Builder (Language)C++ Builder (Language C++)C++ Builder (Compatibility with MFC)C++ Builder (Native API)C++ Builder (Non-Technical)C++ Builder (Students)C++ Builder (Third Party Tools)C++ Builder (Upgrade)C++ Builder (VCL)C++ Builder (VCL Components Usage)C++ Builder (VCL Components Development)C++ Builder (VCL Usage)C++ Builder (WinAPI)C++ Builder Installation Delphi Newsgroups----------------Delphi (General)Delphi ActiveX Controls UsageDelphi ActiveX Controls DevelopmentDelphi As400Delphi DatabasesDelphi Databases (ADO)Delphi Databases (dbExpress)Delphi Databases (Desktop)Delphi Databases (InterBase Express)Delphi Databases (Multi-Tier)Delphi Databases (SQL Servers)Delphi DeploymentDelphi GraphicsDelphi IDEDelphi InternationalizationDelphi InternetDelphi Internet Isapi-WebBrokerDelphi Internet WebSnapDelphi Internet WinsockDelphi JobsDelphi Language BASMDelphi Language ObjectPascalDelphi Model Driven Architecture (General)Delphi Model Driven Architecture (Third Party)Delphi MultimediaDelphi Native APIDelphi Non-TechnicalDelphi ObjectPascalDelphi OLE AutomationDelphi OO designDelphi OpenToolsAPIDelphi Reporting-ChartingDelphi RTLDelphi StudentsDelphi TeamSourceDelphi Thirdparty ToolsDelphi Thirdparty Tools (General)Delphi Thirdparty Tools (IntraWeb)Delphi Thirdparty Tools (RaveReports)Delphi UpgradeDelphi VCLDelphi VCL ComponentsDelphi VCL Components UsingDelphi VCL Components WritingDelphi WebServices SOAPDelphi WinAPIDelphi XMLDelphi Installationcomp.lang.pascal.delphi.components.misccomp.lang.pascal.delphi.components.usagecomp.lang.pascal.delphi.components.writingcomp.lang.pascal.delphi.databasescomp.lang.pascal.delphi.misccomp.sources.delphi JBuilder Newsgroups----------------JBuilderJBuilder AnnouncementsJBuilder Applet IssuesJBuilder As400JBuilder CompilerJBuilder CORBA/RMIJBuilder DatabasesJBuilder DB SwingJBuilder DebuggerJBuilder DeploymentJBuilder DocumentationJBuilder EnterpriseJBuilder HandheldJBuilder IDEJBuilder JavaJBuilder JavaAPIJBuilder Java LanguageJBuilder Java SwingJBuilder Java BeansJBuilder Java Beans UsageJBuilder Java Beans DevelopmentJBuilder JBCLJBuilder JDataStoreJBuilder JobsJBuilder LinuxJBuilder MacintoshJBuilder & MobileSetJBuilder MultiLingual AppsJBuilder Non-TechnicalJBuilder OpenToolsJBuilder Servlets (JSP)JBuilder StudentsJBuilder Team DevelopmentJBuilder Thirdparty ToolsJBuilder UpgradeJBuilder Installation InterBase Newsgroups----------------InterBaseInterBase Bug DiscussionInterBase GeneralInterBase IBConsoleInterBase OpenSourceInterBase ReplicationInterBase SQLInterBase ThirdpartyInterBase Installation Miscellaneous Borland Newsgroups----------------AppCenterAppServerEnteraVisiBroker  You cannot post new topics in this forum You cannot reply to topics in this forum You cannot edit your posts in this forum You cannot delete your posts in this forum You cannot vote in polls in this forum Powered by phpBB © 2001, 2006 phpBB Group SEO toolkit © 2004-2006 webmedic.