Bug 202 - smbclient dies with a bus error on Sparc
smbclient dies with a bus error on Sparc
Status: RESOLVED LATER
Product: Samba 3.0
Classification: Unclassified
Component: smbclient
3.0.0preX
All Linux
: P2 normal
: none
Assigned To: Tim Potter
http://bugs.debian.org/cgi-bin/bugrep...
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-07-03 08:41 UTC by Eloy Paris
Modified: 2005-11-14 09:28 UTC (History)
1 user (show)

See Also:


Attachments
Proposed patch (480 bytes, patch)
2003-07-03 08:52 UTC, Eloy Paris
no flags Details
Ensure argument to convert_string() is aligned. (1.20 KB, patch)
2003-07-07 17:03 UTC, Tim Potter
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eloy Paris 2003-07-03 08:41:26 UTC
smbclient dies with a bus error on the Sparc platform when connecting
to a server. I have:

   unix charset = ISO8859-15

in my smb.conf. If I don't have this option it does not crash, so
this must be related to charset handling (I think my charset is not
configured correctly, but it is nonetheless a bug that smbclient
crashes.)

Here's a backtrace (I used smbclient //server/share, but smbclient -L
server also crashes - server is a Windows 2000 server, BTW):

elparis@turbo:/usr/src/samba-3.0.0beta1$ gdb smbclient
GNU gdb 5.3-debian
[...]
This GDB was configured as "sparc-linux"...
(gdb) set args  //sjc-fs1/WG-A
(gdb) run
Starting program: /usr/bin/smbclient //sjc-fs1/WG-A

Program received signal SIGBUS, Bus error.
0x7019dcb0 in __gconv_get_alias_db () from /lib/libc.so.6
(gdb) where
#0  0x7019dcb0 in __gconv_get_alias_db () from /lib/libc.so.6
#1  0x7019b000 in __gconv_get_alias_db () from /lib/libc.so.6
#2  0x70196204 in iconv_close () from /lib/libc.so.6
#3  0x70195668 in iconv () from /lib/libc.so.6
#4  0x00080cbc in sys_iconv (cd=0x702c3734, inbuf=0x18ad39, 
    inbytesleft=0xefffeb98, outbuf=0xefffeb94, outbytesleft=0xefffeb90)
    at lib/iconv.c:120
#5  0x00080dd8 in smb_iconv (cd=0xe48d8, inbuf=0xefffeb9c, 
    inbytesleft=0xefffeb98, outbuf=0xefffeb94, outbytesleft=0xefffeb90)
    at lib/iconv.c:147
#6  0x00062478 in convert_string (from=936152, to=CH_UNIX, src=0x18ad39, 
    srclen=22, dest=0x179be4, destlen=256) at lib/charcnv.c:160
#7  0x00062e3c in pull_ucs2 (base_ptr=0xe48d8, dest=0x179be4 "", src=0x18ad39, 
    dest_len=256, src_len=22, flags=24) at lib/charcnv.c:631
#8  0x00039ff0 in cli_negprot (cli=0x1796c0) at libsmb/cliconnect.c:968
#9  0x0002776c in do_connect (server=0xefffecf2 "sjc-fs1", 
    share=0x1100 <Address 0x1100 out of bounds>) at client/client.c:2545
#10 0x00027b08 in process (base_directory=0x1796c0 "") at client/client.c:2602
#11 0x000282a4 in main (argc=2, argv=0xeffffb24) at client/client.c:2890
#12 0x70194b98 in __libc_start_main () from /lib/libc.so.6
(gdb)
Comment 1 Eloy Paris 2003-07-03 08:45:50 UTC
I've also opened a but on the Debian Bug Tracking System. The URL is
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=197172. I've added a patch to
the bug report in the Debian BTS but am not sure it is correct. The patch
reverses something that Tridge did a couple of years ago. I read the CVS commit
log that Tridge wrote and can't figure out why he did it.

This bug should affect all platform where it is illegal to write 16- or 32-bit
values to non-aligned memory addresses, i.e. Sparc, MC68000, Alpha.
Comment 2 Eloy Paris 2003-07-03 08:52:38 UTC
Created attachment 42 [details]
Proposed patch

This patch solves the problem on my Sparc. However, we'd like someone from the
Samba Team to take a look at it to make sure it is correct.
Comment 3 Tim Potter 2003-07-06 20:21:22 UTC
Unfortunately the fix is not correct as the workgroup string can sometimes
be sent over the wire starting on an odd byte boundary for this SMB.  

It looks like the convert_string() function is being passed an unaligned address
which is causing the crash.  A better fix would be to align the source string in
convert_string() before passing it to smb_iconv().

For reference, the commit message is shown below.

----------------------------
revision 1.19
date: 2001/06/21 05:38:28;  author: tridge;  state: Exp;  lines: +6 -0

Added STR_NOALIGN flags to clistr and srvstr fns. Yes, NT actually does
send unaligned unicode strings sometimes!
Fixed our handling of the workgroup name tacked on the end of the
NT1 negprot response (a unaligned unicode)
fixed a couple of places where we should be using the message_end fns instead
of pre-calculated buffer lengths
Comment 4 Eloy Paris 2003-07-07 11:02:36 UTC
Hi Tim,

> Unfortunately the fix is not correct as the workgroup string can
> sometimes be sent over the wire starting on an odd byte boundary for
> this SMB.

Yes, I believe this (that the workgroup string can sometimes be sent
over the wire starting on an odd byte boundary) is what causes the
crash.

> It looks like the convert_string() function is being passed an
> unaligned address which is causing the crash. A better fix would be
> to align the source string in convert_string() before passing it to
> smb_iconv().

Well, I am a bit confused (quoting proposed patch):

--- source/libsmb/cliconnect.c.orig     2003-06-12 16:37:05.000000000 -0400
+++ source/libsmb/cliconnect.c  2003-06-12 16:30:17.000000000 -0400
@@ -965,7 +965,7 @@
    smb_buflen(cli->inbuf) > 8) {
       clistr_pull(cli, cli->server_domain,
            smb_buf(cli->inbuf)+8, sizeof(cli->server_domain),
-                   smb_buflen(cli->inbuf)-8, STR_UNICODE|STR_NOALIGN);
+                   smb_buflen(cli->inbuf)-8, STR_UNICODE);

If we set STR_NOALIGN (as the current code is doing) then no attempt
will be made to align the string, which will lead to a read from a
non-aligned address, which will then cause the crash.

If we do not set STR_NOALIGN (as in the patch above) then ucs2_align()
in source/lib/charcnv.c will align the string. Isn't this what we want?
In this case alignment will be performed even if the original string is
aligned, but this shouldn't be a problem, should it?

It seems to me like the meaning of STR_NOALIGN is a bit confusing: it
can be interpreted as "the string is not aligned" or as "do not align the
string". I believe the correct meaning is the latter.
Comment 5 Tim Potter 2003-07-07 17:00:02 UTC
>If we set STR_NOALIGN (as the current code is doing) then no attempt
>will be made to align the string, which will lead to a read from a
>non-aligned address, which will then cause the crash.

Yes.

>If we do not set STR_NOALIGN (as in the patch above) then ucs2_align()
>in source/lib/charcnv.c will align the string. Isn't this what we want?

The string is not aligned in that the address to start copying the string from
in pull_ucs2() is on an odd byte boundary.  If we try to align it by using
STR_NOALIGN, then we are effectively starting to read the string in the middle
of a unicode character.

I suggest that we memcpy() the (unaligned) string from the source buffer to an
aligned buffer, then call convert_string().  This way we are preserving the
non-aligned nature of the source string in the input buffer but preventing the
iconv function from crashing because we are pointing at a non-aligned address.

Does this make sense?  Perhaps some code would better illustrate what I am
trying to say.
Comment 6 Tim Potter 2003-07-07 17:03:05 UTC
Created attachment 44 [details]
Ensure argument to convert_string() is aligned.

Not tested on sparc but still works on i386.  (-:
Comment 7 Gerald (Jerry) Carter 2003-08-28 12:05:36 UTC
so what is the status of this one Tim?
Comment 8 Eloy Paris 2003-08-28 12:31:07 UTC
I am sorry Jerry. Tim has been waiting for me to test the proposed patch on the
Sparc. Been busy but I'll try to do it now.
Comment 9 Gerald (Jerry) Carter 2003-11-04 20:00:50 UTC
will someone do something with this?  Tim, if the patch has 
not been applied and you think it is correct, go ahead an 
apply it.
Comment 10 Tim Potter 2003-11-06 14:18:11 UTC
I was waiting for some feedback from Eloy as to whether the patch worked for him
before applying it to cvs.
Comment 11 Gerald (Jerry) Carter 2003-11-29 21:05:28 UTC
open for too long.  no recent feedback from eloy.  Closing.
Comment 12 Gerald (Jerry) Carter 2005-02-07 08:39:06 UTC
originally reported against 3.0.0beta1.  CLeaning out 
non-production release versions.
Comment 13 Gerald (Jerry) Carter 2005-11-14 09:28:32 UTC
database cleanup