Bug 8606 - Intermittent print job failures caused by character conversion errors
Intermittent print job failures caused by character conversion errors
Status: RESOLVED FIXED
Product: Samba 3.6
Classification: Unclassified
Component: Printing
3.6.1
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks: 8595
  Show dependency treegraph
 
Reported: 2011-11-14 13:13 UTC by David Disseldorp
Modified: 2012-01-16 19:49 UTC (History)
1 user (show)

See Also:
jra: review? (metze)


Attachments
git-am fix for 3.6.next (8.17 KB, patch)
2012-01-14 00:48 UTC, Jeremy Allison
ddiss: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Disseldorp 2011-11-14 13:13:07 UTC
Windows OpenPrinterEx requests have been observed carrying a device mode formname "A4" followed by non-utf16 garbage after the null terminator. Such requests fail during unmarshalling in the ndr_pull_charset() codepath, causing print job failures.

Currently the entire device_mode->formname array is processed for character conversion during unmarshalling. This is unnecessary, only bytes prior to the null terminator need be converted.
Comment 1 Guenther Deschner 2011-11-14 13:25:03 UTC
Can you upload some binary examples of this ?
Comment 2 David Disseldorp 2011-11-14 13:31:09 UTC
(In reply to comment #1)
> Can you upload some binary examples of this ?

I should be able to, I'll need to get permission from the customer first.

In the meantime I've pushed a spoolss torture test and proposed fix to:

http://git.samba.org/?p=ddiss/samba.git;a=shortlog;h=refs/heads/formname_char_conv_rb2

The following changes since commit 72cabbbe50a36986dde823f0ba60abf9052c535a:

  s3:smb2_flush: outbody only needs 4 bytes (2011-11-14 10:01:30 +0100)

are available in the git repository at:
  git://git.samba.org/ddiss/samba.git formname_char_conv_rb2

David Disseldorp (5):
      spoolss: test unmarshalling formname with garbage after null
      ndr: add ndr_pull_charset_to_null()
      idl: add to_null property
      idl: add parser for the to_null property
      idl: add to_null attribute to the spoolss formname array

 librpc/idl/spoolss.idl                   |    2 +-
 librpc/ndr/libndr.h                      |    1 +
 librpc/ndr/ndr_string.c                  |   31 ++++++++++++++++++++++++++++++
 pidl/lib/Parse/Pidl/NDR.pm               |    9 +++++++-
 pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm |    6 ++++-
 source4/torture/rpc/spoolss.c            |   28 +++++++++++++++++++++++++++
 6 files changed, 74 insertions(+), 3 deletions(-)
Comment 3 David Disseldorp 2011-11-23 15:55:43 UTC
Below is an example of garbage after the OpenPrinterEx->DeviceMode->formname and OpenPrinterEx->DeviceMode->devicename null terminators:

[2011/11/22 14:05:13.089754, 10, pid=28264] ../lib/util/util.c:304(_dump_data)
  [0000] 00 5C 00 50 00 49 00 50   00 45 00 5C 00 00 00 00   .\.P.I.P .E.\....
  [0010] 00 05 00 00 03 10 00 00   00 9E 04 00 00 01 00 00   ........ ........
  [0020] 00 86 04 00 00 00 00 45   00 00 00 02 00 14 00 00   .......E ........
  [0030] 00 00 00 00 00 14 00 00   00 5C 00 5C 00 59 00 47   ........ .\.\.Y.G
  [0040] 00 34 00 52 00 39 00 30   00 31 00 33 00 5C 00 4E   .4.R.9.0 .1.3.\.N
  [0050] 00 57 00 44 00 5F 00 34   00 38 00 34 00 31 00 00   .W.D._.4 .8.4.1..
  [0060] 00 04 00 02 00 04 00 00   00 00 00 00 00 04 00 00   ........ ........
  [0070] 00 52 00 41 00 57 00 00   00 B4 03 00 00 08 00 02   .R.A.W.. ........
  [0080] 00 B4 03 00 00 5C 00 5C   00 59 00 47 00 34 00 52   .....\.\ .Y.G.4.R
  [0090] 00 39 00 30 00 31 00 33   00 5C 00 4E 00 57 00 44   .9.0.1.3 .\.N.W.D
  [00A0] 00 5F 00 34 00 38 00 34   00 31 00 00 00 A0 00 BA   ._.4.8.4 .1......
* garbage follows here...
  [00B0] 00 92 01 02 00 38 00 CA   00 92 01 02 00 C8 00 AA   .....8.. ........
  [00C0] 00 92 01 02 00 01 04 02   05 DC 00 D8 02 53 FF 81   ........ .....S..
  [00D0] 03 01 00 09 00 9A 0B 34   08 64 00 01 00 0F 00 58   .......4 .d.....X
  [00E0] 02 02 00 01 00 58 02 02   00 01 00 41 00 34 00 00   .....X.. ...A.4..
* and here...
  [00F0] 00 CB 00 92 01 02 00 78   00 13 20 92 01 02 00 F0   .......x .. .....
  [0100] 00 1E 20 92 01 02 00 18   00 22 20 92 01 02 00 E0   .. ..... ." .....
  [0110] 00 B0 00 92 01 02 00 68   00 AA 00 92 01 02 00 E0   .......h ........
  [0120] 00 92 01 92 01 02 00 38   00 C5 00 00 00 00 00 00   .......8 ........
  [0130] 00 00 00 00 00 00 00 00   00 01 00 00 00 00 00 00   ........ ........
  [0140] 00 01 00 00 00 02 00 00   00 00 01 00 00 00 00 00   ........ ........
  [0150] 00 00 00 00 00 00 00 00   00 00 00 00 00 00 00 00   ........ ........
  [0160] 00 50 52 49 56 E2 30 00   00 00 00 00 00 00 00 00   .PRIV.0. ........
  [0170] 00 00 00 00 00 00 00 00   00 00 00 00 00 00 00 00   ........ ........
  [0180] 00 00 00 00 00 00 00 00   00 00 00 00 00 00 00 00   ........ ........
  [0190] 00 00 00 00 00 00 00 00   00 00 00 00 00 00 00 00   ........ ........
  [01A0] 00 00 00 00 00 00 00 00   00 00 00 00 00 00 00 00   ........ ........
  [01B0] 00 00 00 00 00 00 00 00   00 18 00 00 00 00 00 10   ........ ........
  [01C0] 27 10 27 10 27 00 00 10   27 00 00 00 00 00 00 00   '.'.'... '.......
  [01D0] 00 00 00 C4 02 00 00 00   00 00 00 00 00 00 00 00   ........ ........
  [01E0] 00 00 00 00 00 00 00 00   00 00 00 00 00 03 00 00   ........ ........
  [01F0] 00 00 00 00 00 14 00 10   00 50 34 03 00 28 88 04   ........ .P4..(..
Comment 4 David Disseldorp 2011-11-28 15:34:34 UTC
Changes since comment 2:
- add to_null attribute to the spoolss devicename array
- tweak the existing spoolss ndr torture test, rather
  than the writing new rpc tests to do the same thing.

The following changes since commit 3741cf999e0f05a831d88330ac6bfa7ad34c2ec7:

  Remove unused variable. (2011-11-24 00:17:40 +0100)

are available in the git repository at:
  git://git.samba.org/ddiss/samba.git gd_formname_ndr_test_with_fix_rb1

David Disseldorp (6):
      s4-smbtorture: tweak spoolss_OpenPrinterEx devmode
      idl: add parser for the to_null property
      idl: add to_null property
      ndr: add ndr_pull_charset_to_null()
      idl: add to_null attribute to the spoolss formname array
      idl: add to_null attribute to the spoolss devicename array

 librpc/idl/spoolss.idl                   |    4 +-
 librpc/ndr/libndr.h                      |    1 +
 librpc/ndr/ndr_string.c                  |   31 ++++++++++++++++++++++++++++++
 pidl/lib/Parse/Pidl/NDR.pm               |    9 +++++++-
 pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm |    6 ++++-
 source4/torture/ndr/spoolss.c            |    4 +-
 6 files changed, 49 insertions(+), 6 deletions(-)
Comment 5 Jeremy Allison 2012-01-11 23:53:39 UTC
Ok, this patchset:

http://git.samba.org/?p=ddiss/samba.git;a=shortlog;h=refs/heads/gd_formname_ndr_test_with_fix_rb1

looks correct to me for 3.6.next.

What is the status of this fix ? I'm adding Metze to the CC: list as I recall him making a comment on this.

Jeremy.
Comment 6 Jeremy Allison 2012-01-14 00:48:47 UTC
Created attachment 7240 [details]
git-am fix for 3.6.next

David's patch back-ported for 3.5.next.

Jeremy.
Comment 7 Jeremy Allison 2012-01-14 00:49:12 UTC
Comment on attachment 7240 [details]
git-am fix for 3.6.next

Arg. I meant "back ported for 3.6.next", not 3.5.next.
Comment 8 David Disseldorp 2012-01-14 13:31:18 UTC
Comment on attachment 7240 [details]
git-am fix for 3.6.next

Back-port looks good, thanks Jeremy!
Comment 9 Jeremy Allison 2012-01-16 04:55:37 UTC
Re-assigning to Karolin for inclusion in 3.6.next.

Jeremy.
Comment 10 Karolin Seeger 2012-01-16 19:49:26 UTC
Pushed to v3-6-test.
Closing out bug report.

Thanks!