2009/1/4 <duc.sequere.aut.de.via.dec...@imagemagick.org>: The bug does exist :(
> xmp_profile=StringInfoToString(profile); Even in the latest SVN, StringInfoToString() calls CopyMagickString(), and CopyMagickString() contains this at the very end: while (*p++ != '\0') ; So, this attempt to return the length of the original string still searches for \0 outside of the memory allocated for profile. I propose the following change. 1. Add a new function (let's call it CopyMagickStringBlindly) that copies the string, but doesn't attempt to return the length of the original string. 2. Add documentation to CopyMagickString() that says that it is an error to use it on non-terminated source strings, and that it is a waste of resources to ignore its return value. 3. Replace CopyMagickString() with CopyMagickStringBlindly() where possible. The patch below does just that for string.c and string_.h. For the rest of ImageMagick, use the following sed substitutions: find . -name \*.c | grep -v '\.svn' | grep -v 'string.c' | xargs sed -i 's/CopyMagickString/CopyMagickStringBlindly/g' find . -name \*.c | grep -v '\.svn' | grep -v 'string.c' | xargs sed -i 's/=CopyMagickStringBlindly/=CopyMagickString/g' find . -name \*.c | grep -v '\.svn' | grep -v 'string.c' | xargs sed -i 's/(void) CopyMagickStringBlindly/CopyMagickStringBlindly/g' Test the result by attempting to identify http://www.moeller.ru/upload/content/cordyn.jpg under valgrind. Index: string.c =================================================================== --- string.c (revision 13763) +++ string.c (working copy) @@ -126,7 +126,7 @@ ThrowFatalException(ResourceLimitFatalError,"UnableToAcquireString"); *destination='\0'; if (source != (char *) NULL) - (void) CopyMagickString(destination,source,(length+1)*sizeof(*destination)); + CopyMagickStringBlindly(destination,source,(length+1)*sizeof(*destination)); return(destination); } @@ -225,7 +225,7 @@ sizeof(*destination)); if (*destination == (char *) NULL) ThrowFatalException(ResourceLimitFatalError,"UnableToAcquireString"); - (void) CopyMagickString(*destination,source,(length+1)*sizeof(*destination)); + CopyMagickStringBlindly(*destination,source,(length+1)*sizeof(*destination)); return(*destination); } @@ -589,7 +589,7 @@ string[length]='\0'; file=close(file)-1; string_info=AcquireStringInfo(0); - (void) CopyMagickString(string_info->path,filename,MaxTextExtent); + CopyMagickStringBlindly(string_info->path,filename,MaxTextExtent); string_info->length=length; string_info->datum=(unsigned char *) string; return(string_info); @@ -637,7 +637,7 @@ ThrowFatalException(ResourceLimitFatalError,"UnableToAcquireString"); *destination='\0'; if (source != (char *) NULL) - (void) CopyMagickString(destination,source,(length+1)*sizeof(*destination)); + CopyMagickStringBlindly(destination,source,(length+1)*sizeof(*destination)); return(destination); } @@ -656,6 +656,11 @@ % destination buffer is always null-terminated even if the string must be % truncated. % +% The CopyMagickString method returns the length of the source string. Thus, +% it is an error to call it if the source string is not null-terminated. +% Moreover, it is also a waste of time to call it if you wish to ignore the +% return value. Use CopyMagickStringBlindly() instead in such cases. +% % The format of the CopyMagickString method is: % % size_t CopyMagickString(const char *destination,char *source, @@ -722,6 +727,81 @@ % % % % % % +% C o p y M a g i c k S t r i n g B l i n d l y % +% % +% % +% % +%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% +% +% CopyMagickStringBlindly() copies the source string to the destination +% string. The destination buffer is always null-terminated even if the +% string must be truncated. Unlike CopyMagickString, this function doesn't +% return the length of the original string. +% +% The format of the CopyMagickStringBlindly method is: +% +% size_t CopyMagickStringBlindly(const char *destination,char *source, +% const size_t length) +% +% A description of each parameter follows: +% +% o destination: the destination string. +% +% o source: the source string. +% +% o length: the length of the destination string. +% +*/ +MagickExport void CopyMagickStringBlindly(char *destination,const char *source, + const size_t length) +{ + register char + *q; + + register const char + *p; + + register size_t + n; + + p=source; + q=destination; + for (n=length; n > 4; n-=4) + { + *q=(*p++); + if (*q == '\0') + return; + q++; + *q=(*p++); + if (*q == '\0') + return; + q++; + *q=(*p++); + if (*q == '\0') + return; + q++; + *q=(*p++); + if (*q == '\0') + return; + q++; + } + if (n != 0) + for (n--; n != 0; n--) + { + *q=(*p++); + if (*q == '\0') + return; + q++; + } + if (length != 0) + *q='\0'; +} + +/* +%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% +% % +% % +% % % D e s t r o y S t r i n g % % % % % @@ -963,7 +1043,7 @@ (void) LogMagickEvent(TraceEvent,GetMagickModule(),"%s",filename); assert(exception != (ExceptionInfo *) NULL); string_info=AcquireStringInfo(0); - (void) CopyMagickString(string_info->path,filename,MaxTextExtent); + CopyMagickStringBlindly(string_info->path,filename,MaxTextExtent); string_info->datum=FileToBlob(filename,extent,&string_info->length,exception); if (string_info->datum == (unsigned char *) NULL) { @@ -1729,7 +1809,7 @@ assert(string_info != (StringInfo *) NULL); assert(string_info->signature == MagickSignature); assert(path != (const char *) NULL); - (void) CopyMagickString(string_info->path,path,MaxTextExtent); + CopyMagickStringBlindly(string_info->path,path,MaxTextExtent); } /* @@ -1808,7 +1888,7 @@ if (~length >= MaxTextExtent) string=(char *) AcquireQuantumMemory(length+MaxTextExtent,sizeof(*string)); if (string != (char *) NULL) - (void) CopyMagickString(string,(char *) string_info->datum, + CopyMagickStringBlindly(string,(char *) string_info->datum, (length+1)*sizeof(*string)); return(string); } @@ -1909,7 +1989,7 @@ ThrowFatalException(ResourceLimitFatalError, "UnableToConvertStringToARGV"); } - (void) CopyMagickString(argv[i],p,(size_t) (q-p+1)); + CopyMagickStringBlindly(argv[i],p,(size_t) (q-p+1)); p=q; while ((isspace((int) ((unsigned char) *p)) == 0) && (*p != '\0')) p++; @@ -2181,7 +2261,7 @@ sizeof(*textlist)); if (textlist[i] == (char *) NULL) ThrowFatalException(ResourceLimitFatalError,"UnableToConvertText"); - (void) CopyMagickString(textlist[i],p,(size_t) (q-p+1)); + CopyMagickStringBlindly(textlist[i],p,(size_t) (q-p+1)); if (*q == '\r') q++; p=q+1; @@ -2215,7 +2295,7 @@ for (j=1; j <= (long) MagickMin(strlen(p),0x14); j++) { (void) FormatMagickString(hex_string,MaxTextExtent,"%02x",*(p+j)); - (void) CopyMagickString(q,hex_string,MaxTextExtent); + CopyMagickStringBlindly(q,hex_string,MaxTextExtent); q+=2; if ((j % 0x04) == 0) *q++=' '; @@ -2419,7 +2499,7 @@ ThrowFatalException(ResourceLimitFatalError, "UnableToAcquireString"); } - (void) CopyMagickString(result+destination_offset,source,copy_length+1); + CopyMagickStringBlindly(result+destination_offset,source,copy_length+1); destination_offset+=copy_length; } /* Index: string_.h =================================================================== --- string_.h (revision 13763) +++ string_.h (working copy) @@ -98,6 +98,7 @@ extern MagickExport void ConcatenateStringInfo(StringInfo *,const StringInfo *), + CopyMagickStringBlindly(char *,const char *,const size_t), LocaleLower(char *), LocaleUpper(char *), PrintStringInfo(FILE *file,const char *,const StringInfo *), -- Alexander E. Patrakov _______________________________________________ Magick-developers mailing list Magick-developers@imagemagick.org http://studio.imagemagick.org/mailman/listinfo/magick-developers
