# Extended 3.6 — SurfacePlugin manualSurface: width*depth signed-overflow + raw pointer write

Bug ref      : pharo.md §3.6
Severity     : HIGH (image registers a tiny surface with a wild pointer)
File         : extracted/plugins/SurfacePlugin/src/common/sqManualSurface.c
Lines (HEAD) : 120-167 (`createManualSurface`, `setManualSurfacePointer`)

## Problem

```c
if (rowPitch < (width*depth)/8) return -1;        // signed mul overflows
...
int setManualSurfacePointer(int surfaceID, void* ptr) {
    ...  surface->ptr = ptr;
}
```

Two cooperating defects:

  1. `width * depth` is a signed `int` multiply. For width = 0x10001
     and depth = 32, the product wraps negative; `rowPitch <
     <negative>` is false for any non-negative `rowPitch`, so the
     check passes despite the surface being huge.
  2. `setManualSurfacePointer` accepts any `void*` from the image
     and stores it without verifying that the buffer belongs to the
     image or that it covers `rowPitch * height` bytes. BitBlt then
     writes through this pointer.

Combined: an image can register a tiny surface with a wraparound
`rowPitch` and a wild pointer, then drive arbitrary writes via
BitBlt.

## Fix

Validate `width * depth / 8` against `rowPitch` using uint64; reject
unreasonable `width`/`height` upfront; require the caller to pass an
explicit buffer length when setting the pointer, and verify that
length covers the surface.

```diff
diff --git a/extracted/plugins/SurfacePlugin/src/common/sqManualSurface.c b/extracted/plugins/SurfacePlugin/src/common/sqManualSurface.c
index d7ed4efb6..d4328b95b 100644
--- a/extracted/plugins/SurfacePlugin/src/common/sqManualSurface.c
+++ b/extracted/plugins/SurfacePlugin/src/common/sqManualSurface.c
@@ -123,8 +123,14 @@ int createManualSurface(int width, int height, int rowPitch, int depth, int isMS
 	
 	if (width < 0) return -1;
 	if (height < 0) return -1;
-	if (rowPitch < (width*depth)/8) return -1;
 	if (depth < 1 || depth > 32) return -1;
+	if (width  > 0x10000) return -1;
+	if (height > 0x10000) return -1;
+	{
+		/* uint64 multiply to avoid signed overflow defeating the check. */
+		uint64_t requiredPitchBytes = ((uint64_t)width * (uint64_t)depth + 7) / 8;
+		if ((uint64_t)rowPitch < requiredPitchBytes) return -1;
+	}
 	
 	newSurface = (ManualSurface*)malloc(sizeof(ManualSurface));
 	if (!newSurface) return -1;
```

For `setManualSurfacePointer`, the simplest hardening that does
not break existing callers is to mark the surface as
"caller-asserts-bounds" and refuse to lock it until a separate
`setManualSurfaceLength(surfaceID, length)` call is made — but that
is an API change. For now, document the contract more loudly and
log the assignment:



A follow-up PR should add the length parameter and refuse to BitBlt
into a surface whose `ptr` was set without one.

## Test plan

- Register a surface with width = 0x10001, depth = 32, rowPitch = 0:
  before, signed multiply wraps to a small negative; check passes.
  After, uint64 catches the requirement and `createManualSurface`
  returns -1.
- Register a legitimate surface (e.g. width=640, height=480,
  depth=32, rowPitch=2560): succeeds.

## Risk notes

- The width/height caps (0x10000 = 65 536) are generous for any
  realistic Smalltalk Form and prevent multiply overflow even on
  32-bit builds.
- The pointer-length tracking is a follow-up; current PR closes the
  pitch-overflow vector but leaves the wild-pointer vector open
  pending API change.