Do not use keywords for headers

deadtrickster commented 9 years ago

As I understand keywords are never GCed and blindly interning all unknown names can result in massive memory leak. (Especially this relevant for Hunchentoot obviously, but all software based on chunga potentially vulnerable). Personally I like [and propose] this solution https://github.com/deadtrickster/ia-hash-table.

PS. RFC(http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2) states: Field names are case-insensitive AND as-keyword still calls intern for 'known' names with different case

CL-USER> (chunga::as-keyword "User-Agent")
:USER-AGENT

CL-USER> (chunga::as-keyword "User-agent")
:USER-AGENT
:EXTERNAL

so I don't think documentation string for +string-to-keyword-hash+ is correct.

stassats commented 9 years ago

It is indeed a problem, but sadly nothing can be done about it now, even if not many people use chunga, drakma and hunchentoot provide keywords as part of their interface and changing this will break existing code.

r13l commented 3 years ago

@stassats, I agree that changing the interface is a non-starter; existing code should not be broken. One approach might be to control the safe behaviour with a special variable. Gated this way, there are a number of options I can think of:

The current default: all header names are interned.
Only already-interned header names are mapped to keyword symbols; all others are strings. This is ugly, but it has the advantage of requiring few modifications to existing code. Notably, EQL works when comparing against keyword symbols.
All header names are canonicalised to a single case.

As an example:

* (let ((chunga:*header-strings* :uninterned))
    (flexi-streams:with-input-from-sequence (stream (flexi-streams:string-to-octets "Content-type: text/plain
Never-interned-before: not even once

"))
    (chunga:read-http-headers stream)))
→ ((:CONTENT-TYPE . "text/plain") ("Never-Interned-Before" . "not even once"))

Possible values for *HEADER-STRINGS* might be NIL, :UNINTERNED or :ALL.

r13l commented 3 years ago

The following program illustrates the need for this issue to be addressed:

(ql:quickload '("hunchentoot" "ironclad"))
(hunchentoot:start (make-instance 'hunchentoot:easy-acceptor :port 4242))

;; this will cause the server to intern symbols until there is no more memory available
(let ((sock (make-instance 'sb-bsd-sockets:inet-socket :type :stream :protocol :tcp))
      ss
      (chars "abcdefghijklmnopqrstuvqxyz0123456789"))
  (sb-bsd-sockets:socket-connect sock #(127 0 0 1) 4242)
  (setq ss (sb-bsd-sockets:socket-make-stream sock :output t :input t :buffering :none :element-type 'character :auto-close t))
  (format ss "GET / HTTP/1.0~c~c" #\Return #\Newline)
  (flet ((random-string (&aux (string (make-string (1+ (ironclad:strong-random 128)))))
       (loop for i from 0 below (length string)
         do (setf (aref string i) (aref chars (ironclad:strong-random (length chars)))))
       string))
    (loop (format ss "~a: ~a~c~c" (random-string) (random-string) #\Return #\Newline))
    (format ss "~c~c" #\Return #\Newline)))

edicl / chunga

Do not use keywords for headers #3