Bugzilla

(In reply to Magnus Melin [:mkmelin] from comment #36)
> Seems the crash is one line off for some reason. Must be 642 instead of 641. https://searchfox.org/comm-central/rev/ced14298b38d818a49b90571fb4d8c04a8668c02/mailnews/local/src/nsParseMailbox.cpp#642

I am just curious. Come to think of it, why are we using PL_strncat() instead of strncat()?
I think all reasonable targets (Windows, linux, MacOSX, etc.) today have sane implementation of 
strncat().

I checked the code in PL_strncat() in  https://searchfox.org/comm-central/source/mozilla/nsprpub/lib/libc/src/strcat.c#20
PL_strcat in the same file calls strcat if both the string parameters are non-null. So we might use strcat in the first place here.
PL_strncat eventually calls PL_strncpy() defined in https://searchfox.org/comm-central/source/mozilla/nsprpub/lib/libc/src/strcpy.c#20
If I read correctly PL_strncpy does not seem to terminate the resulting data with nul at all (?!).
Whoa! 
See the commented out section. https://searchfox.org/comm-central/source/mozilla/nsprpub/lib/libc/src/strcpy.c#34
Is this intended behavior? It is rather different from strncat drastically.

Now we can argue that |value| is allocated by |PL_CALLOC()| and should start with NUL-filled state, 
and so "PL_strncpy() called from PL_strncat() not terminating the result with nul when the data is copied" does not matter in THIS PARTICULAR  routine.
But it is very convoluted and can be a maintenance headache. It takes maybe 10 minutes to figure out the internal working of this routine for someone who reads this the first time and verify all the above. It actually takes more than 10 minutes, more like 30 minutes.

Another issue is that we are copying a non-null-terminated strings to an array that is again
handled as a pair of {length, data} in the end, which does not seem to assume null-termination.
So use of bcopy (and its friend) makes it explicit about the nature of data we are handling, and more appropriate here IMHO.

I tried to re-read the code and to be frank, I felt it would be better to use bcopy (and its friend) by keeping track of where to copy the data in each loop.

I suggest rewriting this routine using bcopy (or equivalent such as  memcpy) if we still get the strange crashes in this routine,  

One other reason I suggest rewriting is this routine is not a speed monster. It reminds me of bubble sort.
Its execution time is not O(length), but somewhere between O(length) and O(length ^ 2) because strncat needs to scan the 
area so far filled. 
Since this routine is a concatenation operation  of "to" or "cc" (and "bcc", too?)  addresses, we can say it may not be a big deal unless we have many addresses as mailing list. If we do (like 256 addresses), I think the execution time is large.
But using bcopy (or equivalent) can cut the execution down to O(length) neatly.

Just a thought. 

The behavior of |PL_strncpy()| should be verified if it is intended as such.
Modifying it may not be wise since it seems to be used in so many places.
I think we should replace it with strncpy() (and PL_strncat() with strncat()) where appropriate during maintenance rewrite nearby.

(In reply to Magnus Melin [:mkmelin] from comment #36)
> Seems the crash is one line off for some reason. Must be 642 instead of 641. https://searchfox.org/comm-central/rev/ced14298b38d818a49b90571fb4d8c04a8668c02/mailnews/local/src/nsParseMailbox.cpp#642

I am just curious. Come to think of it, why are we using PL_strncat() instead of strncat()?
I think all reasonable targets (Windows, linux, MacOSX, etc.) today have sane implementation of 
strncat().

I checked the code in PL_strncat() in  https://searchfox.org/comm-central/source/mozilla/nsprpub/lib/libc/src/strcat.c#20
PL_strcat in the same file calls strcat if both the string parameters are non-null. So we might use strcat in the first place here.
PL_strncat eventually calls PL_strncpy() defined in https://searchfox.org/comm-central/source/mozilla/nsprpub/lib/libc/src/strcpy.c#20

--- I misread it. The following is a lie.
If I read correctly PL_strncpy does not seem to terminate the resulting data with nul at all (?!).
Whoa! 
See the commented out section. https://searchfox.org/comm-central/source/mozilla/nsprpub/lib/libc/src/strcpy.c#34
----
BUT it still does not fill the remaining area with nul's as strncat does when the data (is nul-termianted) and its string length is less than the third argument.  This is what strncat is supposed to do.
Is this intended behavior? It is rather different from strncat drastically.


Now we can argue that |value| is allocated by |PL_CALLOC()| and should start with NUL-filled state, 
and so "PL_strncpy() called from PL_strncat() not terminating the result with nul when the data is copied" does not matter in THIS PARTICULAR  routine.
But it is very convoluted and can be a maintenance headache. It takes maybe 10 minutes to figure out the internal working of this routine for someone who reads this the first time and verify all the above. It actually takes more than 10 minutes, more like 30 minutes.

Another issue is that we are copying a non-null-terminated strings to an array that is again
handled as a pair of {length, data} in the end, which does not seem to assume null-termination.
So use of bcopy (and its friend) makes it explicit about the nature of data we are handling, and more appropriate here IMHO.

I tried to re-read the code and to be frank, I felt it would be better to use bcopy (and its friend) by keeping track of where to copy the data in each loop.

I suggest rewriting this routine using bcopy (or equivalent such as  memcpy) if we still get the strange crashes in this routine,  

One other reason I suggest rewriting is this routine is not a speed monster. It reminds me of bubble sort.
Its execution time is not O(length), but somewhere between O(length) and O(length ^ 2) because strncat needs to scan the 
area so far filled. 
Since this routine is a concatenation operation  of "to" or "cc" (and "bcc", too?)  addresses, we can say it may not be a big deal unless we have many addresses as mailing list. If we do (like 256 addresses), I think the execution time is large.
But using bcopy (or equivalent) can cut the execution down to O(length) neatly.

Just a thought. 

The behavior of |PL_strncpy()| should be verified if it is intended as such.
Modifying it may not be wise since it seems to be used in so many places.
I think we should replace it with strncpy() (and PL_strncat() with strncat()) where appropriate during maintenance rewrite nearby.

EDIT: PL_strncat() does end the data with nul where appropriate.
But it DOES NOT repeat the filling with nul as strncat does under some cases. (That part is conditionalized in the source code)  Maybe this is the little speed improvement of PL_strncat over strncat.
I have no idea if there is a code that depends on this behavior. We really should stick to the widely adopted standard for long-term maintenance. So I vote for the use of strncat, strncpy, etc. Or even for strlcpy, etc.

(In reply to Magnus Melin [:mkmelin] from comment #36)
> Seems the crash is one line off for some reason. Must be 642 instead of 641. https://searchfox.org/comm-central/rev/ced14298b38d818a49b90571fb4d8c04a8668c02/mailnews/local/src/nsParseMailbox.cpp#642

I am just curious. Come to think of it, why are we using PL_strncat() instead of strncat()?
I think all reasonable targets (Windows, linux, MacOSX, etc.) today have sane implementation of 
strncat().

I checked the code in PL_strncat() in  https://searchfox.org/comm-central/source/mozilla/nsprpub/lib/libc/src/strcat.c#20
PL_strcat in the same file calls strcat if both the string parameters are non-null. So we might use strcat in the first place here.
PL_strncat eventually calls PL_strncpy() defined in https://searchfox.org/comm-central/source/mozilla/nsprpub/lib/libc/src/strcpy.c#20

--- I misread it. The following is a lie.
If I read correctly PL_strncpy does not seem to terminate the resulting data with nul at all (?!).
Whoa! 
See the commented out section. https://searchfox.org/comm-central/source/mozilla/nsprpub/lib/libc/src/strcpy.c#34
----
BUT it still does not fill the remaining area with nul's as strncat does when the data (is nul-termianted) and its string length is less than the third argument.  This is what strncat is supposed to do.
Is this intended behavior? It is rather different from strncat drastically.


Now we can argue that |value| is allocated by |PL_CALLOC()| and should start with NUL-filled state, 
and so "PL_strncpy() called from PL_strncat() not fill the result with nul when the data is copied as strncpy may do under certain conditions" does not matter in THIS PARTICULAR  routine.
But it is very convoluted and can be a maintenance headache. It takes maybe 10 minutes to figure out the internal working of this routine for someone who reads this the first time and verify all the above. It actually takes more than 10 minutes, more like 30 minutes.

Another issue is that we are copying a non-null-terminated strings to an array that is again
handled as a pair of {length, data} in the end, which does not seem to assume null-termination.
So use of bcopy (and its friend) makes it explicit about the nature of data we are handling, and more appropriate here IMHO.

I tried to re-read the code and to be frank, I felt it would be better to use bcopy (and its friend) by keeping track of where to copy the data in each loop.

I suggest rewriting this routine using bcopy (or equivalent such as  memcpy) if we still get the strange crashes in this routine,  

One other reason I suggest rewriting is this routine is not a speed monster. It reminds me of bubble sort.
Its execution time is not O(length), but somewhere between O(length) and O(length ^ 2) because strncat needs to scan the 
area so far filled. 
Since this routine is a concatenation operation  of "to" or "cc" (and "bcc", too?)  addresses, we can say it may not be a big deal unless we have many addresses as mailing list. If we do (like 256 addresses), I think the execution time is large.
But using bcopy (or equivalent) can cut the execution down to O(length) neatly.

Just a thought. 

The behavior of |PL_strncpy()| should be verified if it is intended as such. (It diverges from strncpy() when the length of second argument as nul-terminated string is shorter than the third argument, I think.)
Modifying it may not be wise since it seems to be used in so many places.
I think we should replace it with strncpy() (and PL_strncat() with strncat()) where appropriate during maintenance rewrite nearby.

EDIT: PL_strncat() does end the data with nul where appropriate.
But it DOES NOT REPEAT THE FILLING with nul as strncat does under some cases. (That part is conditionalized in the source code)  Maybe this is a tiny speed improvement of PL_strncat over strncat in general. In this routine, it is not a win since we ALWAYS copy up to the third argument.
I have no idea if there is a code that depends on this behavior. of PL_strncpy().  We really should stick to the widely adopted standard for long-term maintenance. So I vote for the use of strncat, strncpy, etc. Or even for strlcpy, etc. where appropriate.